From d03025ba1097ec41c90fa0279bafd3ab47e1a546 Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Mon, 1 Jul 2019 16:26:57 -0700 Subject: [PATCH 1/4] Revert "gtestdbus: Properly close server connections" This reverts commit baf92d09d69de0d9f9b2d0f77fc62c21fdef4da8. Closes #787 According to the original commit, this change was made because otherwise g_test_dbus_down() following a g_test_dbus_stop() hangs until it times out. The timeout being referred to is the 30 seconds which are waited by _g_object_unref_and_wait_weak_notify() for the GWeakNotify to be triggered when the last strong reference to the singleton GDBusConnection object is dropped. But the patch was not correct and the leak should have instead been fixed by having the last strong reference holder drop their reference on the GDBusConnection before calling g_test_dbus_down(). Timing out after 30 seconds is the desired behavior in the case where someone holds a reference to the singleton for that entire period. There are a few problems with this patch. First, as pointed out here[1], calling g_object_run_dispose() in the idle callback means we are causing the GWeakNotify to trigger ~immediately rather than waiting 30 seconds to give another owner a chance to unref. Second, since someone else may still hold a reference on the object being disposed, they may call methods on it after it's been disposed which can seg fault as documented here[2] and as I also saw recently in another project. It's unclear what the original leak being fixed was, but many have been fixed between 2013 and now. I ran all the unit tests under valgrind, and some do fail (some consistently and some intermittently) but none of the failures seem to only happen after this reversion commit. I also couldn't find anywhere in the valgrind output where any GDBusConnection objects are definitely being lost. [1] https://gitlab.gnome.org/GNOME/glib/issues/787#note_214226 [2] https://gitlab.gnome.org/GNOME/glib/issues/787#note_214237 --- gio/gtestdbus.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/gio/gtestdbus.c b/gio/gtestdbus.c index 99c9cf8ea..6c060aa7c 100644 --- a/gio/gtestdbus.c +++ b/gio/gtestdbus.c @@ -67,15 +67,14 @@ on_weak_notify_timeout (gpointer user_data) } static gboolean -dispose_on_idle (gpointer object) +unref_on_idle (gpointer object) { - g_object_run_dispose (object); g_object_unref (object); return FALSE; } static gboolean -_g_object_dispose_and_wait_weak_notify (gpointer object) +_g_object_unref_and_wait_weak_notify (gpointer object) { WeakNotifyData data; guint timeout_id; @@ -87,7 +86,7 @@ _g_object_dispose_and_wait_weak_notify (gpointer object) /* Drop the ref in an idle callback, this is to make sure the mainloop * is already running when weak notify happens */ - g_idle_add (dispose_on_idle, object); + g_idle_add (unref_on_idle, object); /* Make sure we don't block forever */ timeout_id = g_timeout_add (30 * 1000, on_weak_notify_timeout, &data); @@ -820,7 +819,7 @@ g_test_dbus_down (GTestDBus *self) stop_daemon (self); if (connection != NULL) - _g_object_dispose_and_wait_weak_notify (connection); + _g_object_unref_and_wait_weak_notify (connection); g_test_dbus_unset (); _g_bus_forget_singleton (G_BUS_TYPE_SESSION); From 1c63d5d539b20e57869a6f73d94256602c2c087d Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Mon, 1 Jul 2019 15:55:10 -0700 Subject: [PATCH 2/4] Revert "Work around test failure in gdbus-names" This reverts commit c37cd19feee3a609fec8909f01df8755052c59ab. Now that we've reverted the commit "gtestdbus: Properly close server connections", g_test_dbus_down() no longer returns early and we no longer need this workaround. Since the gdbus-names test seems to properly unref its GDBusConnection objects it's not clear to me why it needed the sleep to succeed. However even at the time the failure wasn't reproducible according to this comment[1] so it's probably not worth spending more effort trying to reproduce it now. [1] https://gitlab.gnome.org/GNOME/glib/issues/787#note_214235 --- gio/tests/gdbus-names.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/gio/tests/gdbus-names.c b/gio/tests/gdbus-names.c index 648b54774..4f95b457a 100644 --- a/gio/tests/gdbus-names.c +++ b/gio/tests/gdbus-names.c @@ -472,9 +472,6 @@ test_bus_own_name (void) g_object_unref (c2); session_bus_down (); - - /* See https://bugzilla.gnome.org/show_bug.cgi?id=711807 */ - g_usleep (1000000); } /* ---------------------------------------------------------------------------------------------------- */ From 59ce6b10dc93746dcae081d01dbe13a254430b4b Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Mon, 1 Jul 2019 17:12:50 -0700 Subject: [PATCH 3/4] gdbus-connection-loss: Fix test failure Now that we're not calling g_object_run_dispose() indirectly in g_test_dbus_down() (see commit "Revert "gtestdbus: Properly close server connections""), the test gdbus-connection-loss is failing with the message "Bail out! GLib-GIO-FATAL-WARNING: Weak notify timeout, object ref_count=1". This is because we're holding a reference to the singleton connection object while calling session_bus_down() in the test's main(). So then we end up waiting for 30 seconds in _g_object_unref_and_wait_weak_notify() for the GWeakNotify to be triggered, which never happens. The fix is to unref the connection before calling session_bus_down(). This is consistent with how other tests work, and is safe because the only method called on the connection has already errored out, as asserted by the test. --- gio/tests/gdbus-connection-loss.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gio/tests/gdbus-connection-loss.c b/gio/tests/gdbus-connection-loss.c index 9dbbeb2a4..8f7023f3f 100644 --- a/gio/tests/gdbus-connection-loss.c +++ b/gio/tests/gdbus-connection-loss.c @@ -136,9 +136,10 @@ main (int argc, ret = g_test_run(); + g_object_unref (c); + session_bus_down (); - g_object_unref (c); g_main_loop_unref (loop); return ret; From d4db5a828883a040be00dd0f80400132220a1b72 Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Tue, 2 Jul 2019 15:54:52 -0700 Subject: [PATCH 4/4] gtestdbus: Clarify comment on dropping connection ref In _g_object_unref_and_wait_weak_notify() we take a weak reference and then call g_object_unref() in an idle callback, which may look like we're dropping a strong reference without having one. So change the comment to make it more clear that the reference being dropped is held by the caller. --- gio/gtestdbus.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gio/gtestdbus.c b/gio/gtestdbus.c index 6c060aa7c..11cf029d9 100644 --- a/gio/gtestdbus.c +++ b/gio/gtestdbus.c @@ -84,8 +84,9 @@ _g_object_unref_and_wait_weak_notify (gpointer object) g_object_weak_ref (object, (GWeakNotify) g_main_loop_quit, data.loop); - /* Drop the ref in an idle callback, this is to make sure the mainloop - * is already running when weak notify happens */ + /* Drop the strong ref held by the caller in an idle callback. This is to + * make sure the mainloop is already running when weak notify happens (when + * all other strong ref holders have dropped theirs). */ g_idle_add (unref_on_idle, object); /* Make sure we don't block forever */