2006-10-01 Havoc Pennington <hp@redhat.com>

	* test/test-service.c (path_message_func): remove broken extra
	unref that was hidden by the bugs in dbus-connection.c/dbus-bus.c

	* test/test-shell-service.c (path_message_func): same fix

	* dbus/dbus-connection.c
	(_dbus_connection_get_dispatch_status_unlocked): break up the
	function a little for clarity and fix the notification of
	dbus-bus.c to not require dispatch to be complete

	* dbus/dbus-connection.c (dbus_connection_unref): improve the
	warning when you try to finalize an open connection.

3 files changed, 101 insertions, 55 deletions

diff --git a/dbus/dbus-bus.c b/dbus/dbus-bus.c
index f6918fd5..9eb2c526 100644
--- a/dbus/dbus-bus.c
+++ b/dbus/dbus-bus.c
@@ -257,6 +257,10 @@ bus_data_free (void *data)
       int i;
       _DBUS_LOCK (bus);
       /* We may be stored in more than one slot */
+      /* This should now be impossible - these slots are supposed to
+       * be cleared on disconnect, so should not need to be cleared on
+       * finalize
+       */
       i = 0;
       while (i < N_BUS_TYPES)
         {
@@ -427,7 +431,10 @@ internal_bus_get (DBusBusType  type,
 
   if (!private)
     {
-      /* get a weak ref to the connection */
+      /* store a weak ref to the connection (dbus-connection.c is
+       * supposed to have a strong ref that it drops on disconnect,
+       * since this is a shared connection)
+       */
       bus_connections[type] = connection;
     }
   
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 41058751..a94e0b2e 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -1964,7 +1964,8 @@ _dbus_connection_last_unref (DBusConnection *connection)
  * For shared connections, libdbus will own a reference
  * as long as the connection is connected, so you can know that either
  * you don't have the last reference, or it's OK to drop the last reference.
- * Most connections are shared.
+ * Most connections are shared. dbus_connection_open() and dbus_bus_get()
+ * return shared connections.
  *
  * For private connections, the creator of the connection must arrange for
  * dbus_connection_close() to be called prior to dropping the last reference.
@@ -2002,7 +2003,20 @@ dbus_connection_unref (DBusConnection *connection)
 #endif
   
   if (last_unref)
-    _dbus_connection_last_unref (connection);
+    {
+#ifndef DBUS_DISABLE_CHECKS
+      if (_dbus_transport_get_is_connected (connection->transport))
+        {
+          _dbus_warn ("The last reference on a connection was dropped without closing the connection. This is a bug. See dbus_connection_unref() documentation for details.\n");
+          if (connection->shareable)
+            _dbus_warn ("Most likely, the application called unref() too many times and removed a reference belonging to libdbus, since this is a shared connection.\n");
+          else
+            _dbus_warn ("Most likely, the application was supposed to call dbus_connection_close(), since this is a private connection.\n");
+          return;
+        }
+#endif
+      _dbus_connection_last_unref (connection);
+    }
 }
 
 /*
@@ -2109,6 +2123,7 @@ dbus_connection_close (DBusConnection *connection)
 
   CONNECTION_LOCK (connection);
 
+#ifndef DBUS_DISABLE_CHECKS
   if (connection->shareable)
     {
       CONNECTION_UNLOCK (connection);
@@ -2116,7 +2131,8 @@ dbus_connection_close (DBusConnection *connection)
       _dbus_warn ("Applications must not close shared connections - see dbus_connection_close() docs. This is a bug in the application.\n");
       return;
     }
-
+#endif
+  
   _dbus_connection_close_possibly_shared_and_unlock (connection);
 }
 
@@ -3670,6 +3686,67 @@ _dbus_connection_failed_pop (DBusConnection *connection,
   connection->n_incoming += 1;
 }
 
+/* Note this may be called multiple times since we don't track whether we already did it */
+static void
+notify_disconnected_unlocked (DBusConnection *connection)
+{
+  HAVE_LOCK_CHECK (connection);
+
+  /* Set the weakref in dbus-bus.c to NULL, so nobody will get a disconnected
+   * connection from dbus_bus_get(). We make the same guarantee for
+   * dbus_connection_open() but in a different way since we don't want to
+   * unref right here; we instead check for connectedness before returning
+   * the connection from the hash.
+   */
+  _dbus_bus_notify_shared_connection_disconnected_unlocked (connection);
+
+  /* Dump the outgoing queue, we aren't going to be able to
+   * send it now, and we'd like accessors like
+   * dbus_connection_get_outgoing_size() to be accurate.
+   */
+  if (connection->n_outgoing > 0)
+    {
+      DBusList *link;
+      
+      _dbus_verbose ("Dropping %d outgoing messages since we're disconnected\n",
+                     connection->n_outgoing);
+      
+      while ((link = _dbus_list_get_last_link (&connection->outgoing_messages)))
+        {
+          _dbus_connection_message_sent (connection, link->data);
+        }
+    } 
+}
+
+/* Note this may be called multiple times since we don't track whether we already did it */
+static DBusDispatchStatus
+notify_disconnected_and_dispatch_complete_unlocked (DBusConnection *connection)
+{
+  HAVE_LOCK_CHECK (connection);
+  
+  if (connection->disconnect_message_link != NULL)
+    {
+      _dbus_verbose ("Sending disconnect message from %s\n",
+                     _DBUS_FUNCTION_NAME);
+      
+      /* If we have pending calls, queue their timeouts - we want the Disconnected
+       * to be the last message, after these timeouts.
+       */
+      connection_timeout_and_complete_all_pending_calls_unlocked (connection);
+      
+      /* We haven't sent the disconnect message already,
+       * and all real messages have been queued up.
+       */
+      _dbus_connection_queue_synthesized_message_link (connection,
+                                                       connection->disconnect_message_link);
+      connection->disconnect_message_link = NULL;
+
+      return DBUS_DISPATCH_DATA_REMAINS;
+    }
+
+  return DBUS_DISPATCH_COMPLETE;
+}
+
 static DBusDispatchStatus
 _dbus_connection_get_dispatch_status_unlocked (DBusConnection *connection)
 {
@@ -3692,59 +3769,21 @@ _dbus_connection_get_dispatch_status_unlocked (DBusConnection *connection)
       
       if (!is_connected)
         {
-          /* It's possible this would be better done by having an
-           * explicit notification from
-           * _dbus_transport_disconnect() that would synchronously
-           * do this, instead of waiting for the next dispatch
-           * status check. However, probably not good to change
-           * until it causes a problem.
+          /* It's possible this would be better done by having an explicit
+           * notification from _dbus_transport_disconnect() that would
+           * synchronously do this, instead of waiting for the next dispatch
+           * status check. However, probably not good to change until it causes
+           * a problem.
            */
-          
-          if (status == DBUS_DISPATCH_COMPLETE &&
-              connection->disconnect_message_link)
-            {              
-              _dbus_verbose ("Sending disconnect message from %s\n",
-                             _DBUS_FUNCTION_NAME);
-          
-              /* If we have pending calls, queue their timeouts - we want the Disconnected
-               * to be the last message, after these timeouts.
-               */
-              connection_timeout_and_complete_all_pending_calls_unlocked (connection);
-              
-              /* We haven't sent the disconnect message already,
-               * and all real messages have been queued up.
-               */
-              _dbus_connection_queue_synthesized_message_link (connection,
-                                                               connection->disconnect_message_link);
-              connection->disconnect_message_link = NULL;
-
-              /* Set the weakref in dbus-bus.c to NULL, so nobody will get a disconnected
-               * connection from dbus_bus_get(). We make the same guarantee for
-               * dbus_connection_open() but in a different way since we don't want to
-               * unref right here; we instead check for connectedness before returning
-               * the connection from the hash.
-               */
-              _dbus_bus_notify_shared_connection_disconnected_unlocked (connection);
-              
-              status = DBUS_DISPATCH_DATA_REMAINS;
-            }
+          notify_disconnected_unlocked (connection);
 
-          /* Dump the outgoing queue, we aren't going to be able to
-           * send it now, and we'd like accessors like
-           * dbus_connection_get_outgoing_size() to be accurate.
+          /* I'm not sure this is needed; the idea is that we want to
+           * queue the Disconnected only after we've read all the
+           * messages, but if we're disconnected maybe we are guaranteed
+           * to have read them all ?
            */
-          if (connection->n_outgoing > 0)
-            {
-              DBusList *link;
-              
-              _dbus_verbose ("Dropping %d outgoing messages since we're disconnected\n",
-                             connection->n_outgoing);
-              
-              while ((link = _dbus_list_get_last_link (&connection->outgoing_messages)))
-                {
-                  _dbus_connection_message_sent (connection, link->data);
-                }
-            }
+          if (status == DBUS_DISPATCH_COMPLETE)
+            status = notify_disconnected_and_dispatch_complete_unlocked (connection);
         }
       
       if (status != DBUS_DISPATCH_COMPLETE)
diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c
index e8c03ef3..b7040abf 100644
--- a/dbus/dbus-sysdeps.c
+++ b/dbus/dbus-sysdeps.c
@@ -69,7 +69,7 @@ _dbus_abort (void)
     {
       /* don't use _dbus_warn here since it can _dbus_abort() */
       fprintf (stderr, "  Process %lu sleeping for gdb attach\n", (unsigned long) _dbus_getpid());
-      _dbus_sleep_milliseconds (1000 * 60);
+      _dbus_sleep_milliseconds (1000 * 180);
     }
   
   abort ();