gdbus: Unref cached GDBusConnection objects when the connection is closed

This change fixes the dbus-leak tests by dropping the cached
GDBusConnection objects references when the bus connection is closed.

The issue was introduced with recent changes made to GLib[1]
where invoking g_test_dbus_down() will fail after a
timeout if the GDBusConnection object for the session bus leaks.

Given g_test_dbus_down() will first close the connection before checking
for leaks unreffing the object when the connection is closed should fix
the issue.

[1] https://gitlab.gnome.org/GNOME/glib/merge_requests/963

Signed-off-by: Andre Moreira Magalhaes <andre@endlessm.com>

4 files changed, 174 insertions, 57 deletions

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 a8985cd..f80bdb7 100644
--- a/gdbus/dconf-gdbus-thread.c
+++ b/gdbus/dconf-gdbus-thread.c
@@ -190,18 +190,32 @@ dconf_gdbus_get_bus_common (GBusType   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,
                                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
@@ -326,6 +345,8 @@ static GDBusConnection *
 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
@@ -344,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 *
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,