Merge branch 'fix-dbus-leaks' into 'master'

Fix dbus leaks

See merge request GNOME/dconf!51

7 files changed, 246 insertions, 74 deletions

diff --git a/engine/dconf-engine.h b/engine/dconf-engine.h
index 2485423..6d57782 100644
--- a/engine/dconf-engine.h
+++ b/engine/dconf-engine.h
@@ -67,6 +67,17 @@ GVariant *              dconf_engine_dbus_call_sync_func                (GBusTyp
                                                                          const GVariantType      *expected_type,
                                                                          GError                 **error);
 
+/* Helper function used by the client library to handle bus disconnection */
+G_GNUC_INTERNAL
+void                    dconf_engine_dbus_handle_connection_closed      (GDBusConnection         *connection,
+                                                                         gboolean                 remote_peer_vanished,
+                                                                         GError                  *error,
+                                                                         GMutex                  *bus_lock,
+                                                                         gboolean                *bus_is_error,
+                                                                         gpointer                *bus_data,
+                                                                         GCallback                bus_closed_callback,
+                                                                         gpointer                 bus_closed_callback_user_data);
+
 /* Notifies that a change occured.
  *
  * The engine lock is never held when calling this function so it is
diff --git a/gdbus/dconf-gdbus-common.c b/gdbus/dconf-gdbus-common.c
new file mode 100644
index 0000000..4b1b00c
--- /dev/null
+++ b/gdbus/dconf-gdbus-common.c
@@ -0,0 +1,42 @@
+#include "../engine/dconf-engine.h"
+
+void
+dconf_engine_dbus_handle_connection_closed (GDBusConnection *connection,
+                                            gboolean         remote_peer_vanished,
+                                            GError          *error,
+                                            GMutex          *bus_lock,
+                                            gboolean        *bus_is_error,
+                                            gpointer        *bus_data,
+                                            GCallback        bus_closed_callback,
+                                            gpointer         bus_closed_callback_user_data)
+{
+  g_return_if_fail (connection != NULL);
+  g_return_if_fail (bus_is_error != NULL);
+  g_return_if_fail (bus_data != NULL);
+
+  g_debug ("D-Bus connection closed, invalidating cache: %s",
+           error != NULL ? error->message :
+             (remote_peer_vanished == FALSE ? "Close requested" : "Unknown reason"));
+
+  g_mutex_lock (bus_lock);
+
+  if (bus_closed_callback)
+    g_signal_handlers_disconnect_by_func (connection,
+                                          bus_closed_callback,
+                                          bus_closed_callback_user_data);
+
+  if (*bus_is_error)
+    {
+      g_clear_error ((GError **) bus_data);
+      *bus_is_error = FALSE;
+    }
+  else
+    {
+      g_assert (connection == *bus_data);
+      *bus_data = NULL;
+    }
+
+  g_object_unref (connection);
+
+  g_mutex_unlock (bus_lock);
+}
diff --git a/gdbus/dconf-gdbus-filter.c b/gdbus/dconf-gdbus-filter.c
index 79b2dd7..dddc00c 100644
--- a/gdbus/dconf-gdbus-filter.c
+++ b/gdbus/dconf-gdbus-filter.c
@@ -6,7 +6,7 @@
 typedef struct
 {
   gpointer data; /* either GDBusConnection or GError */
-  guint    is_error;
+  gboolean is_error;
   guint    waiting_for_serial;
   GQueue   queue;
 } ConnectionState;
@@ -156,6 +156,22 @@ dconf_gdbus_filter_function (GDBusConnection *connection,
   return message;
 }
 
+static void
+dconf_gdbus_bus_connection_closed (GDBusConnection *connection,
+                                   gboolean         remote_peer_vanished,
+                                   GError          *error,
+                                   gpointer         user_data)
+{
+  ConnectionState *state = user_data;
+
+  dconf_engine_dbus_handle_connection_closed (connection, remote_peer_vanished, error,
+                                              &dconf_gdbus_lock,
+                                              &state->is_error,
+                                              &state->data,
+                                              G_CALLBACK (dconf_gdbus_bus_connection_closed),
+                                              user_data);
+}
+
 static ConnectionState *
 dconf_gdbus_get_connection_state (GBusType   bus_type,
                                   GError   **error)
@@ -166,7 +182,7 @@ dconf_gdbus_get_connection_state (GBusType   bus_type,
 
   state = &connections[bus_type];
 
-  if (g_once_init_enter (&state->data))
+  if (!state->data)
     {
       GDBusConnection *connection;
       GError *error = NULL;
@@ -180,6 +196,9 @@ dconf_gdbus_get_connection_state (GBusType   bus_type,
 
       if (connection)
         {
+          g_signal_connect (connection, "closed",
+                            G_CALLBACK (dconf_gdbus_bus_connection_closed),
+                            state);
           g_dbus_connection_add_filter (connection, dconf_gdbus_filter_function, state, NULL);
           result = connection;
           state->is_error = FALSE;
@@ -190,11 +209,11 @@ dconf_gdbus_get_connection_state (GBusType   bus_type,
           state->is_error = TRUE;
         }
 
-      g_once_init_leave (&state->data, result);
+      state->data = result;
     }
 
   if (!connection_state_ensure_success (state, error))
-    return FALSE;
+    return NULL;
 
   return state;
 }
@@ -213,62 +232,67 @@ dconf_engine_dbus_call_async_func (GBusType                bus_type,
   GDBusMessage *message;
   DConfGDBusCall *call;
   gboolean success;
+  volatile guint *serial_ptr;
+  guint my_serial;
+
+  /* Hold the lock to make sure nothing else updates state (e.g. invalidates
+   * connection) while we are processing here.
+   */
+  g_mutex_lock (&dconf_gdbus_lock);
 
   state = dconf_gdbus_get_connection_state (bus_type, error);
 
   if (state == NULL)
     {
       g_variant_unref (g_variant_ref_sink (parameters));
+
+      g_mutex_unlock (&dconf_gdbus_lock);
+
       return FALSE;
     }
 
   message = g_dbus_message_new_method_call (bus_name, object_path, interface_name, method_name);
   g_dbus_message_set_body (message, parameters);
 
-  g_mutex_lock (&dconf_gdbus_lock);
-  {
-    volatile guint *serial_ptr;
-    guint my_serial;
-
-    /* We need to set the serial in call->serial.  Sometimes we also
-     * need to set it in state->waiting_for_serial (in the case that no
-     * other items are queued yet).
-     *
-     * g_dbus_connection_send_message() only has one out_serial parameter
-     * so we can only set one of them atomically.  If needed, we elect
-     * to set the waiting_for_serial because that is the one that is
-     * accessed from the filter function without holding the lock.
-     *
-     * The serial number in the call structure is only accessed after the
-     * lock is acquired which allows us to take our time setting it (for
-     * as long as we're still holding the lock).
-     *
-     * In the case that waiting_for_serial should not be set we just use
-     * a local variable and use that to fill call->serial.
-     *
-     * Also: the queue itself isn't accessed until after the lock is
-     * taken, so we can delay adding the call to the queue until we know
-     * that the sending of the message was successful.
-     */
-
-    if (g_queue_is_empty (&state->queue))
-      serial_ptr = &state->waiting_for_serial;
-    else
-      serial_ptr = &my_serial;
-
-    success = g_dbus_connection_send_message (connection_state_get_connection (state), message,
-                                              G_DBUS_SEND_MESSAGE_FLAGS_NONE, serial_ptr, error);
+  /* We need to set the serial in call->serial.  Sometimes we also
+   * need to set it in state->waiting_for_serial (in the case that no
+   * other items are queued yet).
+   *
+   * g_dbus_connection_send_message() only has one out_serial parameter
+   * so we can only set one of them atomically.  If needed, we elect
+   * to set the waiting_for_serial because that is the one that is
+   * accessed from the filter function without holding the lock.
+   *
+   * The serial number in the call structure is only accessed after the
+   * lock is acquired which allows us to take our time setting it (for
+   * as long as we're still holding the lock).
+   *
+   * In the case that waiting_for_serial should not be set we just use
+   * a local variable and use that to fill call->serial.
+   *
+   * Also: the queue itself isn't accessed until after the lock is
+   * taken, so we can delay adding the call to the queue until we know
+   * that the sending of the message was successful.
+   */
+
+  if (g_queue_is_empty (&state->queue))
+    serial_ptr = &state->waiting_for_serial;
+  else
+    serial_ptr = &my_serial;
+
+  success = g_dbus_connection_send_message (connection_state_get_connection (state), message,
+                                            G_DBUS_SEND_MESSAGE_FLAGS_NONE, serial_ptr, error);
+
+  if (success)
+    {
+      call = g_slice_new (DConfGDBusCall);
 
-    if (success)
-      {
-        call = g_slice_new (DConfGDBusCall);
+      call->handle = handle;
+      call->serial = *serial_ptr;
 
-        call->handle = handle;
-        call->serial = *serial_ptr;
+      g_queue_push_tail (&state->queue, call);
+    }
 
-        g_queue_push_tail (&state->queue, call);
-      }
-  }
   g_mutex_unlock (&dconf_gdbus_lock);
 
   g_object_unref (message);
@@ -286,18 +310,35 @@ dconf_engine_dbus_call_sync_func (GBusType             bus_type,
                                   const GVariantType  *reply_type,
                                   GError             **error)
 {
+  g_autoptr(GDBusConnection) connection = NULL;
   ConnectionState *state;
 
+  /* Hold the lock to make sure nothing else updates state (e.g. invalidates
+   * connection) while we are processing here.
+   */
+  g_mutex_lock (&dconf_gdbus_lock);
+
   state = dconf_gdbus_get_connection_state (bus_type, error);
 
   if (state == NULL)
     {
       g_variant_unref (g_variant_ref_sink (parameters));
 
+      g_mutex_unlock (&dconf_gdbus_lock);
+
       return NULL;
     }
 
-  return g_dbus_connection_call_sync (connection_state_get_connection (state),
+  /* Hold a ref to the connection to make sure the object is still
+   * valid even if we get a closed signal on it (in which case the
+   * actual call would fail but the object would still be alive)
+   * while processing here.
+   */
+  connection = g_object_ref (connection_state_get_connection (state));
+
+  g_mutex_unlock (&dconf_gdbus_lock);
+
+  return g_dbus_connection_call_sync (connection,
                                       bus_name, object_path, interface_name, method_name, parameters, reply_type,
                                       G_DBUS_CALL_FLAGS_NONE, -1, NULL, error);
 }
diff --git a/gdbus/dconf-gdbus-thread.c b/gdbus/dconf-gdbus-thread.c
index 8b8f048..f80bdb7 100644
--- a/gdbus/dconf-gdbus-thread.c
+++ b/gdbus/dconf-gdbus-thread.c
@@ -176,32 +176,46 @@ static gpointer dconf_gdbus_get_bus_data[5];
 static gboolean dconf_gdbus_get_bus_is_error[5];
 
 static GDBusConnection *
-dconf_gdbus_get_bus_common (GBusType       bus_type,
-                            const GError **error)
+dconf_gdbus_get_bus_common (GBusType   bus_type,
+                            GError   **error)
 {
   if (dconf_gdbus_get_bus_is_error[bus_type])
     {
       if (error)
-        *error = dconf_gdbus_get_bus_data[bus_type];
+        *error = g_error_copy (dconf_gdbus_get_bus_data[bus_type]);
 
       return NULL;
     }
 
-  return dconf_gdbus_get_bus_data[bus_type];
+  return g_object_ref (dconf_gdbus_get_bus_data[bus_type]);
+}
+
+static void
+dconf_gdbus_bus_connection_closed (GDBusConnection *connection,
+                                   gboolean         remote_peer_vanished,
+                                   GError          *error,
+                                   gpointer         user_data)
+{
+  GBusType bus_type = GPOINTER_TO_INT (user_data);
+
+  dconf_engine_dbus_handle_connection_closed (connection, remote_peer_vanished, error,
+                                              &dconf_gdbus_get_bus_lock,
+                                              &dconf_gdbus_get_bus_is_error[bus_type],
+                                              &dconf_gdbus_get_bus_data[bus_type],
+                                              G_CALLBACK (dconf_gdbus_bus_connection_closed),
+                                              user_data);
 }
 
 static GDBusConnection *
-dconf_gdbus_get_bus_in_worker (GBusType       bus_type,
-                               const GError **error)
+dconf_gdbus_get_bus_in_worker (GBusType   bus_type,
+                               GError   **error)
 {
+  GDBusConnection *connection;
   g_assert_cmpint (bus_type, <, G_N_ELEMENTS (dconf_gdbus_get_bus_data));
 
-  /* We're in the worker thread and only the worker thread can ever set
-   * this variable so there is no need to take a lock.
-   */
+  g_mutex_lock (&dconf_gdbus_get_bus_lock);
   if (dconf_gdbus_get_bus_data[bus_type] == NULL)
     {
-      GDBusConnection *connection;
       GError *error = NULL;
       gpointer result;
 
@@ -209,6 +223,9 @@ dconf_gdbus_get_bus_in_worker (GBusType       bus_type,
 
       if (connection)
         {
+          g_signal_connect (connection, "closed",
+                            G_CALLBACK (dconf_gdbus_bus_connection_closed),
+                            GINT_TO_POINTER (bus_type));
           g_dbus_connection_signal_subscribe (connection, NULL, "ca.desrt.dconf.Writer",
                                               NULL, NULL, NULL, G_DBUS_SIGNAL_FLAGS_NO_MATCH_RULE,
                                               dconf_gdbus_signal_handler, GINT_TO_POINTER (bus_type), NULL);
@@ -231,13 +248,15 @@ dconf_gdbus_get_bus_in_worker (GBusType       bus_type,
        * flushed all outstanding writes.  The other CPU has to acquire
        * the lock so it cannot have done any out-of-order reads either.
        */
-      g_mutex_lock (&dconf_gdbus_get_bus_lock);
       dconf_gdbus_get_bus_data[bus_type] = result;
-      g_cond_broadcast (&dconf_gdbus_get_bus_cond);
-      g_mutex_unlock (&dconf_gdbus_get_bus_lock);
     }
 
-  return dconf_gdbus_get_bus_common (bus_type, error);
+  connection = dconf_gdbus_get_bus_common (bus_type, error);
+
+  g_cond_broadcast (&dconf_gdbus_get_bus_cond);
+  g_mutex_unlock (&dconf_gdbus_get_bus_lock);
+
+  return connection;
 }
 
 static void
@@ -260,8 +279,8 @@ static gboolean
 dconf_gdbus_method_call (gpointer user_data)
 {
   DConfGDBusCall *call = user_data;
-  GDBusConnection *connection;
-  const GError *error = NULL;
+  g_autoptr(GDBusConnection) connection = NULL;
+  g_autoptr(GError) error = NULL;
 
   connection = dconf_gdbus_get_bus_in_worker (call->bus_type, &error);
 
@@ -315,16 +334,19 @@ static gboolean
 dconf_gdbus_summon_bus (gpointer user_data)
 {
   GBusType bus_type = GPOINTER_TO_INT (user_data);
+  g_autoptr(GDBusConnection) connection = NULL;
 
-  dconf_gdbus_get_bus_in_worker (bus_type, NULL);
+  connection = dconf_gdbus_get_bus_in_worker (bus_type, NULL);
 
   return G_SOURCE_REMOVE;
 }
 
 static GDBusConnection *
-dconf_gdbus_get_bus_for_sync (GBusType       bus_type,
-                              const GError **error)
+dconf_gdbus_get_bus_for_sync (GBusType   bus_type,
+                              GError   **error)
 {
+  g_autoptr(GDBusConnection) connection = NULL;
+
   g_assert_cmpint (bus_type, <, G_N_ELEMENTS (dconf_gdbus_get_bus_data));
 
   /* I'm not 100% sure we have to lock as much as we do here, but let's
@@ -343,9 +365,10 @@ dconf_gdbus_get_bus_for_sync (GBusType       bus_type,
       while (dconf_gdbus_get_bus_data[bus_type] == NULL)
         g_cond_wait (&dconf_gdbus_get_bus_cond, &dconf_gdbus_get_bus_lock);
     }
+  connection = dconf_gdbus_get_bus_common (bus_type, error);
   g_mutex_unlock (&dconf_gdbus_get_bus_lock);
 
-  return dconf_gdbus_get_bus_common (bus_type, error);
+  return g_steal_pointer (&connection);
 }
 
 GVariant *
@@ -358,18 +381,14 @@ dconf_engine_dbus_call_sync_func (GBusType             bus_type,
                                   const GVariantType  *reply_type,
                                   GError             **error)
 {
-  const GError *inner_error = NULL;
-  GDBusConnection *connection;
+  g_autoptr(GDBusConnection) connection = NULL;
 
-  connection = dconf_gdbus_get_bus_for_sync (bus_type, &inner_error);
+  connection = dconf_gdbus_get_bus_for_sync (bus_type, error);
 
   if (connection == NULL)
     {
       g_variant_unref (g_variant_ref_sink (parameters));
 
-      if (error)
-        *error = g_error_copy (inner_error);
-
       return NULL;
     }
 
diff --git a/gdbus/meson.build b/gdbus/meson.build
index 4fbf3ec..aad517a 100644
--- a/gdbus/meson.build
+++ b/gdbus/meson.build
@@ -1,6 +1,14 @@
+libdconf_gdbus_common_sources = files(
+  'dconf-gdbus-common.c',
+)
+
+libdconf_gdbus_thread_sources = libdconf_gdbus_common_sources + files(
+  'dconf-gdbus-thread.c',
+)
+
 libdconf_gdbus_thread = static_library(
   'dconf-gdbus-thread',
-  sources: 'dconf-gdbus-thread.c',
+  sources: libdconf_gdbus_thread_sources,
   include_directories: top_inc,
   dependencies: libdconf_engine_dep,
   c_args: dconf_c_args,
@@ -12,9 +20,13 @@ libdconf_gdbus_thread_dep = declare_dependency(
   link_with: libdconf_gdbus_thread,
 )
 
+libdconf_gdbus_filter_sources = libdconf_gdbus_common_sources + files(
+  'dconf-gdbus-filter.c',
+)
+
 libdconf_gdbus_filter = static_library(
   'dconf-gdbus-filter',
-  sources: 'dconf-gdbus-filter.c',
+  sources: libdconf_gdbus_filter_sources,
   include_directories: top_inc,
   dependencies: libdconf_engine_dep,
   c_args: dconf_c_args,
diff --git a/tests/dbus-leak.c b/tests/dbus-leak.c
new file mode 100644
index 0000000..f2a366b
--- /dev/null
+++ b/tests/dbus-leak.c
@@ -0,0 +1,45 @@
+#include "../engine/dconf-engine.h"
+
+static void
+test_engine_dbus_call (void)
+{
+  GError *error = NULL;
+  GVariant *reply;
+
+  /* Force a call to the engine to make sure at least one GDBusConnection
+   * is cached.
+   */
+  reply = dconf_engine_dbus_call_sync_func (G_BUS_TYPE_SESSION,
+                                            "org.freedesktop.DBus", "/", "org.freedesktop.DBus", "ListNames",
+                                            g_variant_new ("()"), G_VARIANT_TYPE ("(as)"), &error);
+  g_assert_no_error (error);
+  g_assert (reply != NULL);
+  g_assert (g_variant_is_of_type (reply, G_VARIANT_TYPE ("(as)")));
+  g_variant_unref (reply);
+}
+
+int
+main (int argc, char **argv)
+{
+  GTestDBus *test_bus;
+  int res;
+
+  g_test_init (&argc, &argv, NULL);
+
+  dconf_engine_dbus_init_for_testing ();
+
+  g_test_add_func (DBUS_BACKEND "/dbus/engine-dbus-call", test_engine_dbus_call);
+
+  test_bus = g_test_dbus_new (G_TEST_DBUS_NONE);
+
+  g_test_dbus_up (test_bus);
+
+  res = g_test_run ();
+
+  /* g_test_dbus_down will fail if GDBusConnection leaks */
+  g_test_dbus_down (test_bus);
+
+  g_object_unref (test_bus);
+
+  return res;
+}
diff --git a/tests/meson.build b/tests/meson.build
index 8aa5837..0d4260f 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -28,6 +28,8 @@ unit_tests = [
   ['gvdb', 'gvdb.c', '-DSRCDIR="@0@"'.format(test_dir), libgvdb_dep, []],
   ['gdbus-thread', 'dbus.c', '-DDBUS_BACKEND="/gdbus/thread"', libdconf_gdbus_thread_dep, []],
   ['gdbus-filter', 'dbus.c', '-DDBUS_BACKEND="/gdbus/filter"', libdconf_gdbus_filter_dep, []],
+  ['gdbus-thread-leak', 'dbus-leak.c', '-DDBUS_BACKEND="/gdbus/thread"', [libdconf_client_dep, libdconf_gdbus_thread_dep], []],
+  ['gdbus-filter-leak', 'dbus-leak.c', '-DDBUS_BACKEND="/gdbus/filter"', [libdconf_client_dep, libdconf_gdbus_filter_dep], []],
   ['engine', 'engine.c', '-DSRCDIR="@0@"'.format(test_dir), [dl_dep, libdconf_engine_test_dep, m_dep], libdconf_mock],
   ['client', 'client.c', '-DSRCDIR="@0@"'.format(test_dir), [libdconf_client_dep, libdconf_engine_dep], libdconf_mock],
   ['writer', 'writer.c', '-DSRCDIR="@0@"'.format(test_dir), [glib_dep, dl_dep, m_dep, libdconf_service_dep], [libdconf_mock]],