GDBusConnection: Use correct GMainContext when invoking free functions

Without this fix, the ./gdbus-connection test case occasionally fails, see

 https://bugzilla.gnome.org/show_bug.cgi?id=629945#c5

like this

 /gdbus/connection/basic: OK
 /gdbus/connection/life-cycle: **
ERROR:gdbus-connection.c:223:test_connection_life_cycle: assertion failed:
(!quit_mainloop_fired)
 cleaning up bus with pid 21794
 Aborted (core dumped)

because the callback didn't happen on the same thread as where we are
running the loop.

Signed-off-by: David Zeuthen <davidz@redhat.com>

diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 6a1cc9ec0c5b5569acfecfa36ce197a6d9234685..da2b9f99723764ae254428aeaf820d29cc6d934d 100644 (file)
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -180,6 +180,77 @@ static GDBusConnection *the_system_bus = NULL;
 
 /* ---------------------------------------------------------------------------------------------------- */
 
+typedef struct
+{
+  GDestroyNotify              callback;
+  gpointer                    user_data;
+  GMainContext               *context;
+} CallDestroyNotifyData;
+
+static gboolean
+call_destroy_notify_data_in_idle (gpointer user_data)
+{
+  CallDestroyNotifyData *data = user_data;
+  data->callback (data->user_data);
+  return FALSE;
+}
+
+static void
+call_destroy_notify_data_free (CallDestroyNotifyData *data)
+{
+  if (data->context != NULL)
+    g_main_context_unref (data->context);
+  g_free (data);
+}
+
+/*
+ * call_destroy_notify: <internal>
+ * @context: A #GMainContext or %NULL.
+ * @callback: A #GDestroyNotify or %NULL.
+ * @user_data: Data to pass to @callback.
+ *
+ * Schedules @callback to run in @context.
+ */
+static void
+call_destroy_notify (GMainContext  *context,
+                     GDestroyNotify callback,
+                     gpointer       user_data)
+{
+  if (callback == NULL)
+    goto out;
+
+  if (context == g_main_context_get_thread_default ())
+    {
+      callback (user_data);
+    }
+  else
+    {
+      GSource *idle_source;
+      CallDestroyNotifyData *data;
+
+      data = g_new0 (CallDestroyNotifyData, 1);
+      data->callback = callback;
+      data->user_data = user_data;
+      data->context = context;
+      if (data->context != NULL)
+        g_main_context_ref (data->context);
+
+      idle_source = g_idle_source_new ();
+      g_source_set_priority (idle_source, G_PRIORITY_DEFAULT);
+      g_source_set_callback (idle_source,
+                             call_destroy_notify_data_in_idle,
+                             data,
+                             (GDestroyNotify) call_destroy_notify_data_free);
+      g_source_attach (idle_source, data->context);
+      g_source_unref (idle_source);
+    }
+
+ out:
+  ;
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
+
 static gboolean
 _g_strv_has_string (const gchar* const *haystack,
                     const gchar        *needle)
@@ -3234,8 +3305,9 @@ g_dbus_connection_signal_unsubscribe (GDBusConnection *connection,
     {
       SignalSubscriber *subscriber;
       subscriber = &(g_array_index (subscribers, SignalSubscriber, n));
-      if (subscriber->user_data_free_func != NULL)
-        subscriber->user_data_free_func (subscriber->user_data);
+      call_destroy_notify (subscriber->context,
+                           subscriber->user_data_free_func,
+                           subscriber->user_data);
       if (subscriber->context != NULL)
         g_main_context_unref (subscriber->context);
     }
@@ -3481,8 +3553,9 @@ purge_all_signal_subscriptions (GDBusConnection *connection)
     {
       SignalSubscriber *subscriber;
       subscriber = &(g_array_index (subscribers, SignalSubscriber, n));
-      if (subscriber->user_data_free_func != NULL)
-        subscriber->user_data_free_func (subscriber->user_data);
+      call_destroy_notify (subscriber->context,
+                           subscriber->user_data_free_func,
+                           subscriber->user_data);
       if (subscriber->context != NULL)
         g_main_context_unref (subscriber->context);
     }
@@ -3564,9 +3637,9 @@ exported_interface_free (ExportedInterface *ei)
 {
   g_dbus_interface_info_unref ((GDBusInterfaceInfo *) ei->interface_info);
 
-  if (ei->user_data_free_func != NULL)
-    /* TODO: push to thread-default mainloop */
-    ei->user_data_free_func (ei->user_data);
+  call_destroy_notify (ei->context,
+                       ei->user_data_free_func,
+                       ei->user_data);
 
   if (ei->context != NULL)
     g_main_context_unref (ei->context);
@@ -5249,9 +5322,9 @@ struct ExportedSubtree
 static void
 exported_subtree_free (ExportedSubtree *es)
 {
-  if (es->user_data_free_func != NULL)
-    /* TODO: push to thread-default mainloop */
-    es->user_data_free_func (es->user_data);
+  call_destroy_notify (es->context,
+                       es->user_data_free_func,
+                       es->user_data);
 
   if (es->context != NULL)
     g_main_context_unref (es->context);

diff --git a/gio/tests/gdbus-connection.c b/gio/tests/gdbus-connection.c
index e74474d3edbd343acb9a5c382debee8d41f1c284..866e27cec5e04221c91aa0b4b632cec2da75f9ab 100644 (file)
--- a/gio/tests/gdbus-connection.c
+++ b/gio/tests/gdbus-connection.c
@@ -32,10 +32,36 @@
 /* all tests rely on a shared mainloop */
 static GMainLoop *loop = NULL;
 
+G_GNUC_UNUSED static void
+_log (const gchar *format, ...)
+{
+  GTimeVal now;
+  time_t now_time;
+  struct tm *now_tm;
+  gchar time_buf[128];
+  gchar *str;
+  va_list var_args;
+
+  va_start (var_args, format);
+  str = g_strdup_vprintf (format, var_args);
+  va_end (var_args);
+
+  g_get_current_time (&now);
+  now_time = (time_t) now.tv_sec;
+  now_tm = localtime (&now_time);
+  strftime (time_buf, sizeof time_buf, "%H:%M:%S", now_tm);
+
+  g_print ("%s.%06d: %s\n",
+           time_buf, (gint) now.tv_usec / 1000,
+           str);
+  g_free (str);
+}
+
 static gboolean
 test_connection_quit_mainloop (gpointer user_data)
 {
-  gboolean *quit_mainloop_fired = user_data;
+  volatile gboolean *quit_mainloop_fired = user_data;
+  //_log ("quit_mainloop_fired");
   *quit_mainloop_fired = TRUE;
   g_main_loop_quit (loop);
   return TRUE;
@@ -85,9 +111,9 @@ on_name_owner_changed (GDBusConnection *connection,
 static void
 a_gdestroynotify_that_sets_a_gboolean_to_true_and_quits_loop (gpointer user_data)
 {
-  gboolean *val = user_data;
+  volatile gboolean *val = user_data;
   *val = TRUE;
-
+  //_log ("destroynotify fired for %p", val);
   g_main_loop_quit (loop);
 }
 
@@ -98,10 +124,10 @@ test_connection_life_cycle (void)
   GDBusConnection *c;
   GDBusConnection *c2;
   GError *error;
-  gboolean on_signal_registration_freed_called;
-  gboolean on_filter_freed_called;
-  gboolean on_register_object_freed_called;
-  gboolean quit_mainloop_fired;
+  volatile gboolean on_signal_registration_freed_called;
+  volatile gboolean on_filter_freed_called;
+  volatile gboolean on_register_object_freed_called;
+  volatile gboolean quit_mainloop_fired;
   guint quit_mainloop_id;
   guint registration_id;
 
@@ -182,13 +208,13 @@ test_connection_life_cycle (void)
                                       NULL,                   /* arg0 */
                                       G_DBUS_SIGNAL_FLAGS_NONE,
                                       on_name_owner_changed,
-                                      &on_signal_registration_freed_called,
+                                      (gpointer) &on_signal_registration_freed_called,
                                       a_gdestroynotify_that_sets_a_gboolean_to_true_and_quits_loop);
   /* filter func */
   on_filter_freed_called = FALSE;
   g_dbus_connection_add_filter (c2,
                                 some_filter_func,
-                                &on_filter_freed_called,
+                                (gpointer) &on_filter_freed_called,
                                 a_gdestroynotify_that_sets_a_gboolean_to_true_and_quits_loop);
   /* object registration */
   on_register_object_freed_called = FALSE;
@@ -197,7 +223,7 @@ test_connection_life_cycle (void)
                                                        "/foo",
                                                        (GDBusInterfaceInfo *) &boo_interface_info,
                                                        &boo_vtable,
-                                                       &on_register_object_freed_called,
+                                                       (gpointer) &on_register_object_freed_called,
                                                        a_gdestroynotify_that_sets_a_gboolean_to_true_and_quits_loop,
                                                        &error);
   g_assert_no_error (error);
@@ -205,7 +231,14 @@ test_connection_life_cycle (void)
   /* ok, finalize the connection and check that all the GDestroyNotify functions are invoked as expected */
   g_object_unref (c2);
   quit_mainloop_fired = FALSE;
-  quit_mainloop_id = g_timeout_add (30000, test_connection_quit_mainloop, &quit_mainloop_fired);
+  quit_mainloop_id = g_timeout_add (30000, test_connection_quit_mainloop, (gpointer) &quit_mainloop_fired);
+  //_log ("destroynotifies for\n"
+  //      " register_object %p\n"
+  //      " filter          %p\n"
+  //      " signal          %p",
+  //      &on_register_object_freed_called,
+  //      &on_filter_freed_called,
+  //      &on_signal_registration_freed_called);
   while (TRUE)
     {
       if (on_signal_registration_freed_called &&
@@ -214,7 +247,9 @@ test_connection_life_cycle (void)
         break;
       if (quit_mainloop_fired)
         break;
+      //_log ("entering loop");
       g_main_loop_run (loop);
+      //_log ("exiting loop");
     }
   g_source_remove (quit_mainloop_id);
   g_assert (on_signal_registration_freed_called);