libnm: fix another leak when cleaning up NMClient

We now move the deletion of the context-busy-watcher to and idle handler
on the D-Bus GMainContext.

Note that the idle source does not take an additional reference on the
context. Hence, in certain cases it might happen that the context will
be completely unrefed before the idle handler runs. In that case, we
would leak the object.

Avoid that, by taking an additional reference to the GMainContext.

Note that the alternative would be to unref the context-busy-watcher
via the GSource's GDestroyNotify. That is not done, because then the
busy watcher might be unrefed in a different thread. Instead, we want
that to happen for the right context. The only minor downside of this
is that the user now always pays the price and must iterate the context
to fully clean up. But note that the user anyway must be prepared to
iterate the context after NMClient is gone. And that depends on some
unpredictable events that the user cannot control. That means, either
the user handles this correctly already, or the problem anyway exists
(randomly).

Of course all of the discussed "problems" are very specific. In practice, the
users uses the g_main_context_default() instance and anyway will either
keep iterating it or quit the process after the NMClient instance goes
away.

1 files changed, 31 insertions, 5 deletions

diff --git a/libnm/nm-client.c b/libnm/nm-client.c
index 89f4a02827..66a36adee3 100644
--- a/libnm/nm-client.c
+++ b/libnm/nm-client.c
@@ -7569,6 +7569,18 @@ constructed (GObject *object)
 	NML_NMCLIENT_LOG_D (self, "new NMClient instance");
 }
 
+static inline gboolean
+_dispose_cleanup_context_busy_watcher_cb (gpointer user_data)
+{
+	nm_auto_unref_gmaincontext GMainContext *context = NULL;
+	gs_unref_object GObject *context_busy_watcher = NULL;
+
+	nm_utils_user_data_unpack (user_data, &context, &context_busy_watcher);
+
+	nm_assert (G_IS_OBJECT (context_busy_watcher));
+	return G_SOURCE_REMOVE;
+}
+
 static void
 dispose (GObject *object)
 {
@@ -7621,8 +7633,8 @@ dispose (GObject *object)
 	    && priv->dbus_context) {
 		GSource *cleanup_source;
 
-		/* Technically, we cancelled all pending actions (and these actions keep
-		 * the context_busy_watcher object alive). Also, we passed
+		/* Technically, we cancelled all pending actions (and these actions
+		 * (GTask) keep the context_busy_watcher object alive). Also, we passed
 		 * no destroy notify to g_dbus_connection_signal_subscribe().
 		 * That means, there should be no other unaccounted GSource'es left.
 		 *
@@ -7634,10 +7646,24 @@ dispose (GObject *object)
 		 *
 		 * So to be really sure all this is given, always schedule one last
 		 * cleanup idle action with low priority. This should be the last
-		 * thing related to this instance that keeps the context busy. */
+		 * thing related to this instance that keeps the context busy.
+		 *
+		 * Note that we could also *not* take a reference on priv->dbus_context
+		 * and unref priv->context_busy_watcher via the GDestroyNotify. That would
+		 * allow for the context to be wrapped up early, and when the last user
+		 * gives up the reference to the context, the destroy notify could complete
+		 * without even invoke the idle handler. However, that destroy notify may
+		 * not be called in the right thread. So, we want to be sure that we unref
+		 * the context-busy-watcher in the right context. Hence, we always take an
+		 * additional reference and always cleanup in the idle handler. This means:
+		 * the user *MUST* always keep iterating the context after NMClient got destroyed.
+		 * But that is not a severe limitation, because the user anyway must be prepared
+		 * to do that. That is because in many cases it is necessary anyway (and the user
+		 * wouldn't know a priory when not). This way, it is just always necessary. */
 		cleanup_source = nm_g_idle_source_new (G_PRIORITY_LOW + 10,
-		                                       nm_source_func_unref_gobject,
-		                                       g_steal_pointer (&priv->context_busy_watcher),
+		                                       _dispose_cleanup_context_busy_watcher_cb,
+		                                       nm_utils_user_data_pack (g_main_context_ref (priv->dbus_context),
+		                                                                g_steal_pointer (&priv->context_busy_watcher)),
 		                                       NULL);
 		g_source_attach (cleanup_source, priv->dbus_context);
 		g_source_unref (cleanup_source);