Merge branch 'gsource-dispose' into 'master'

Implement a dispose function for GSource

See merge request GNOME/glib!1175

gio/gcancellable.c

@@ -642,20 +642,19 @@ typedef struct {
   GSource       source;
 
   GCancellable *cancellable;
-  guint         cancelled_handler;
+  gulong        cancelled_handler;
 } GCancellableSource;
 
 /*
- * We can't guarantee that the source still has references, so we are
+ * The reference count of the GSource might be 0 at this point but it is not
- * relying on the fact that g_source_set_ready_time() no longer makes
+ * finalized yet and its dispose function did not run yet, or otherwise we
- * assertions about the reference count - the source might be in the
+ * would have disconnected the signal handler already and due to the signal
- * window between last-unref and finalize, during which its refcount
+ * emission lock it would be impossible to call the signal handler at that
- * is officially 0. However, we *can* guarantee that it's OK to
+ * point. That is: at this point we either have a fully valid GSource, or
- * dereference it in a limited way, because we know we haven't yet reached
+ * it's not disposed or finalized yet and we can still resurrect it as needed.
- * cancellable_source_finalize() - if we had, then we would have waited
+ *
- * for signal emission to finish, then disconnected the signal handler
+ * As such we first ensure that we have a strong reference to the GSource in
- * under the lock.
+ * here before calling any other GSource API.
- * See https://bugzilla.gnome.org/show_bug.cgi?id=791754
  */
 static void
 cancellable_source_cancelled (GCancellable *cancellable,
@@ -663,7 +662,9 @@ cancellable_source_cancelled (GCancellable *cancellable,
 {
   GSource *source = user_data;
 
+  g_source_ref (source);
   g_source_set_ready_time (source, 0);
+  g_source_unref (source);
 }
 
 static gboolean
@@ -679,15 +680,15 @@ cancellable_source_dispatch (GSource     *source,
 }
 
 static void
-cancellable_source_finalize (GSource *source)
+cancellable_source_dispose (GSource *source)
 {
   GCancellableSource *cancellable_source = (GCancellableSource *)source;
 
   if (cancellable_source->cancellable)
     {
-      g_cancellable_disconnect (cancellable_source->cancellable,
+      g_clear_signal_handler (&cancellable_source->cancelled_handler,
-                                cancellable_source->cancelled_handler);
+                              cancellable_source->cancellable);
-      g_object_unref (cancellable_source->cancellable);
+      g_clear_object (&cancellable_source->cancellable);
     }
 }
 
@@ -720,7 +721,7 @@ static GSourceFuncs cancellable_source_funcs =
   NULL,
   NULL,
   cancellable_source_dispatch,
-  cancellable_source_finalize,
+  NULL,
   (GSourceFunc)cancellable_source_closure_callback,
 };
 
@@ -750,6 +751,7 @@ g_cancellable_source_new (GCancellable *cancellable)
 
   source = g_source_new (&cancellable_source_funcs, sizeof (GCancellableSource));
   g_source_set_name (source, "GCancellable");
+  g_source_set_dispose_function (source, cancellable_source_dispose);
   cancellable_source = (GCancellableSource *)source;
 
   if (cancellable)

glib/gmain.c

@@ -361,6 +361,8 @@ struct _GSourcePrivate
    * let it remain empty on Windows) to avoid #ifdef all over the place.
    */
   GSList *fds;
+
+  GSourceDisposeFunc dispose;
 };
 
 typedef struct _GSourceIter
@@ -926,6 +928,40 @@ g_source_new (GSourceFuncs *source_funcs,
   return source;
 }
 
+/**
+ * g_source_set_dispose_function:
+ * @source: A #GSource to set the dispose function on
+ * @dispose: #GSourceDisposeFunc to set on the source
+ *
+ * Set @dispose as dispose function on @source. @dispose will be called once
+ * the reference count of @source reaches 0 but before any of the state of the
+ * source is freed, especially before the finalize function is called.
+ *
+ * This means that at this point @source is still a valid #GSource and it is
+ * allow for the reference count to increase again until @dispose returns.
+ *
+ * The dispose function can be used to clear any "weak" references to the
+ * @source in other data structures in a thread-safe way where it is possible
+ * for another thread to increase the reference count of @source again while
+ * it is being freed.
+ *
+ * The finalize function can not be used for this purpose as at that point
+ * @source is already partially freed and not valid anymore.
+ *
+ * This should only ever be called from #GSource implementations.
+ *
+ * Since: 2.64
+ **/
+void
+g_source_set_dispose_function (GSource            *source,
+			       GSourceDisposeFunc  dispose)
+{
+  g_return_if_fail (source != NULL);
+  g_return_if_fail (source->priv->dispose == NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
+  source->priv->dispose = dispose;
+}
+
 /* Holds context's lock */
 static void
 g_source_iter_init (GSourceIter  *iter,
@@ -1185,6 +1221,8 @@ g_source_attach (GSource      *source,
 {
   guint result = 0;
 
+  g_return_val_if_fail (source != NULL, 0);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, 0);
   g_return_val_if_fail (source->context == NULL, 0);
   g_return_val_if_fail (!SOURCE_DESTROYED (source), 0);
   
@@ -1282,6 +1320,7 @@ g_source_destroy (GSource *source)
   GMainContext *context;
   
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   
   context = source->context;
   
@@ -1313,6 +1352,7 @@ g_source_get_id (GSource *source)
   guint result;
   
   g_return_val_if_fail (source != NULL, 0);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, 0);
   g_return_val_if_fail (source->context != NULL, 0);
 
   LOCK_CONTEXT (source->context);
@@ -1342,6 +1382,8 @@ g_source_get_id (GSource *source)
 GMainContext *
 g_source_get_context (GSource *source)
 {
+  g_return_val_if_fail (source != NULL, NULL);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, NULL);
   g_return_val_if_fail (source->context != NULL || !SOURCE_DESTROYED (source), NULL);
 
   return source->context;
@@ -1373,6 +1415,7 @@ g_source_add_poll (GSource *source,
   GMainContext *context;
   
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   g_return_if_fail (fd != NULL);
   g_return_if_fail (!SOURCE_DESTROYED (source));
   
@@ -1409,6 +1452,7 @@ g_source_remove_poll (GSource *source,
   GMainContext *context;
   
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   g_return_if_fail (fd != NULL);
   g_return_if_fail (!SOURCE_DESTROYED (source));
   
@@ -1459,7 +1503,9 @@ g_source_add_child_source (GSource *source,
   GMainContext *context;
 
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   g_return_if_fail (child_source != NULL);
+  g_return_if_fail (g_atomic_int_get (&child_source->ref_count) > 0);
   g_return_if_fail (!SOURCE_DESTROYED (source));
   g_return_if_fail (!SOURCE_DESTROYED (child_source));
   g_return_if_fail (child_source->context == NULL);
@@ -1520,7 +1566,9 @@ g_source_remove_child_source (GSource *source,
   GMainContext *context;
 
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   g_return_if_fail (child_source != NULL);
+  g_return_if_fail (g_atomic_int_get (&child_source->ref_count) > 0);
   g_return_if_fail (child_source->priv->parent_source == source);
   g_return_if_fail (!SOURCE_DESTROYED (source));
   g_return_if_fail (!SOURCE_DESTROYED (child_source));
@@ -1603,6 +1651,7 @@ g_source_set_callback_indirect (GSource              *source,
   GSourceCallbackFuncs *old_cb_funcs;
   
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   g_return_if_fail (callback_funcs != NULL || callback_data == NULL);
 
   context = source->context;
@@ -1665,6 +1714,7 @@ g_source_set_callback (GSource        *source,
   GSourceCallback *new_callback;
 
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
 
   TRACE (GLIB_SOURCE_SET_CALLBACK (source, func, data, notify));
 
@@ -1778,6 +1828,7 @@ g_source_set_priority (GSource  *source,
   GMainContext *context;
 
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   g_return_if_fail (source->priv->parent_source == NULL);
 
   context = source->context;
@@ -1801,6 +1852,7 @@ gint
 g_source_get_priority (GSource *source)
 {
   g_return_val_if_fail (source != NULL, 0);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, 0);
 
   return source->priority;
 }
@@ -1843,15 +1895,7 @@ g_source_set_ready_time (GSource *source,
   GMainContext *context;
 
   g_return_if_fail (source != NULL);
-  /* We deliberately don't check for ref_count > 0 here, because that
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
-   * breaks cancellable_source_cancelled() in GCancellable: it has no
-   * way to find out that the last-unref has happened until the
-   * finalize() function is called, but that's too late, because the
-   * ref_count already has already reached 0 before that time.
-   * However, priv is only poisoned (set to NULL) after finalize(),
-   * so we can use this as a simple guard against use-after-free.
-   * See https://bugzilla.gnome.org/show_bug.cgi?id=791754 */
-  g_return_if_fail (source->priv != NULL);
 
   context = source->context;
 
@@ -1895,6 +1939,7 @@ gint64
 g_source_get_ready_time (GSource *source)
 {
   g_return_val_if_fail (source != NULL, -1);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, -1);
 
   return source->priv->ready_time;
 }
@@ -1916,6 +1961,7 @@ g_source_set_can_recurse (GSource  *source,
   GMainContext *context;
   
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
 
   context = source->context;
 
@@ -1944,6 +1990,7 @@ gboolean
 g_source_get_can_recurse (GSource  *source)
 {
   g_return_val_if_fail (source != NULL, FALSE);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, FALSE);
   
   return (source->flags & G_SOURCE_CAN_RECURSE) != 0;
 }
@@ -1980,6 +2027,7 @@ g_source_set_name (GSource    *source,
   GMainContext *context;
 
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
 
   context = source->context;
 
@@ -2015,6 +2063,7 @@ const char *
 g_source_get_name (GSource *source)
 {
   g_return_val_if_fail (source != NULL, NULL);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, NULL);
 
   return source->name;
 }
@@ -2071,6 +2120,9 @@ GSource *
 g_source_ref (GSource *source)
 {
   g_return_val_if_fail (source != NULL, NULL);
+  /* We allow ref_count == 0 here to allow the dispose function to resurrect
+   * the GSource if needed */
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) >= 0, NULL);
 
   g_atomic_int_inc (&source->ref_count);
 
@@ -2094,6 +2146,28 @@ g_source_unref_internal (GSource      *source,
 
   if (g_atomic_int_dec_and_test (&source->ref_count))
     {
+      /* If there's a dispose function, call this first */
+      if (source->priv->dispose)
+        {
+          /* Temporarily increase the ref count again so that GSource methods
+           * can be called from dispose(). */
+          g_atomic_int_inc (&source->ref_count);
+          if (context)
+            UNLOCK_CONTEXT (context);
+          source->priv->dispose (source);
+          if (context)
+            LOCK_CONTEXT (context);
+
+          /* Now the reference count might be bigger than 0 again, in which
+           * case we simply return from here before freeing the source */
+          if (!g_atomic_int_dec_and_test (&source->ref_count))
+            {
+              if (!have_lock && context)
+                UNLOCK_CONTEXT (context);
+              return;
+            }
+        }
+
       TRACE (GLIB_SOURCE_BEFORE_FREE (source, context,
                                       source->source_funcs->finalize));
 
@@ -2114,6 +2188,8 @@ g_source_unref_internal (GSource      *source,
 
       if (source->source_funcs->finalize)
 	{
+          gint old_ref_count;
+
           /* Temporarily increase the ref count again so that GSource methods
            * can be called from finalize(). */
           g_atomic_int_inc (&source->ref_count);
@@ -2122,11 +2198,14 @@ g_source_unref_internal (GSource      *source,
 	  source->source_funcs->finalize (source);
 	  if (context)
 	    LOCK_CONTEXT (context);
-          g_atomic_int_add (&source->ref_count, -1);
+          old_ref_count = g_atomic_int_add (&source->ref_count, -1);
+          g_warn_if_fail (old_ref_count == 1);
 	}
 
       if (old_cb_funcs)
         {
+          gint old_ref_count;
+
           /* Temporarily increase the ref count again so that GSource methods
            * can be called from callback_funcs.unref(). */
           g_atomic_int_inc (&source->ref_count);
@@ -2137,7 +2216,8 @@ g_source_unref_internal (GSource      *source,
 
           if (context)
             LOCK_CONTEXT (context);
-          g_atomic_int_add (&source->ref_count, -1);
+          old_ref_count = g_atomic_int_add (&source->ref_count, -1);
+          g_warn_if_fail (old_ref_count == 1);
         }
 
       g_free (source->name);
@@ -2181,6 +2261,7 @@ void
 g_source_unref (GSource *source)
 {
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
 
   g_source_unref_internal (source, source->context, FALSE);
 }
@@ -2484,6 +2565,7 @@ g_source_add_unix_fd (GSource      *source,
   GPollFD *poll_fd;
 
   g_return_val_if_fail (source != NULL, NULL);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, NULL);
   g_return_val_if_fail (!SOURCE_DESTROYED (source), NULL);
 
   poll_fd = g_new (GPollFD, 1);
@@ -2537,6 +2619,7 @@ g_source_modify_unix_fd (GSource      *source,
   GPollFD *poll_fd;
 
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   g_return_if_fail (g_slist_find (source->priv->fds, tag));
 
   context = source->context;
@@ -2574,6 +2657,7 @@ g_source_remove_unix_fd (GSource  *source,
   GPollFD *poll_fd;
 
   g_return_if_fail (source != NULL);
+  g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0);
   g_return_if_fail (g_slist_find (source->priv->fds, tag));
 
   context = source->context;
@@ -2622,6 +2706,7 @@ g_source_query_unix_fd (GSource  *source,
   GPollFD *poll_fd;
 
   g_return_val_if_fail (source != NULL, 0);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, 0);
   g_return_val_if_fail (g_slist_find (source->priv->fds, tag), 0);
 
   poll_fd = tag;
@@ -3052,6 +3137,8 @@ g_main_current_source (void)
 gboolean
 g_source_is_destroyed (GSource *source)
 {
+  g_return_val_if_fail (source != NULL, TRUE);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, TRUE);
   return SOURCE_DESTROYED (source);
 }
 
@@ -4452,6 +4539,8 @@ g_source_get_time (GSource *source)
   GMainContext *context;
   gint64 result;
 
+  g_return_val_if_fail (source != NULL, 0);
+  g_return_val_if_fail (g_atomic_int_get (&source->ref_count) > 0, 0);
   g_return_val_if_fail (source->context != NULL, 0);
 
   context = source->context;

glib/gmain.h

@@ -204,6 +204,20 @@ typedef gboolean (*GSourceFunc)       (gpointer user_data);
 typedef void     (*GChildWatchFunc)   (GPid     pid,
                                        gint     status,
                                        gpointer user_data);
+
+
+/**
+ * GSourceDisposeFunc:
+ * @source: #GSource that is currently being disposed
+ *
+ * Dispose function for @source. See g_source_set_dispose_function() for
+ * details.
+ *
+ * Since: 2.64
+ */
+GLIB_AVAILABLE_TYPE_IN_2_64
+typedef void (*GSourceDisposeFunc)       (GSource *source);
+
 struct _GSource
 {
   /*< private >*/
@@ -536,6 +550,13 @@ GMainContext *g_main_loop_get_context (GMainLoop    *loop);
 GLIB_AVAILABLE_IN_ALL
 GSource *g_source_new             (GSourceFuncs   *source_funcs,
                                    guint           struct_size);
+
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
+GLIB_AVAILABLE_IN_2_64
+void     g_source_set_dispose_function (GSource            *source,
+                                        GSourceDisposeFunc  dispose);
+G_GNUC_END_IGNORE_DEPRECATIONS
+
 GLIB_AVAILABLE_IN_ALL
 GSource *g_source_ref             (GSource        *source);
 GLIB_AVAILABLE_IN_ALL

glib/tests/mainloop.c

@@ -25,25 +25,29 @@
 #include <stdio.h>
 #include <string.h>
 
-static gboolean cb (gpointer data)
+static gboolean
+cb (gpointer data)
 {
   return FALSE;
 }
 
-static gboolean prepare (GSource *source, gint *time)
+static gboolean
+prepare (GSource *source, gint *time)
 {
   return FALSE;
 }
-static gboolean check (GSource *source)
+static gboolean
+check (GSource *source)
 {
   return FALSE;
 }
-static gboolean dispatch (GSource *source, GSourceFunc cb, gpointer date)
+static gboolean
+dispatch (GSource *source, GSourceFunc cb, gpointer date)
 {
   return FALSE;
 }
 
-GSourceFuncs funcs = {
+static GSourceFuncs funcs = {
   prepare,
   check,
   dispatch,
@@ -1746,6 +1750,82 @@ test_nfds (void)
   g_main_context_unref (ctx);
 }
 
+static gboolean source_finalize_called = FALSE;
+static guint source_dispose_called = 0;
+static gboolean source_dispose_recycle = FALSE;
+
+static void
+finalize (GSource *source)
+{
+  g_assert_false (source_finalize_called);
+  source_finalize_called = TRUE;
+}
+
+static void
+dispose (GSource *source)
+{
+  /* Dispose must always be called before finalize */
+  g_assert_false (source_finalize_called);
+
+  if (source_dispose_recycle)
+    g_source_ref (source);
+  source_dispose_called++;
+}
+
+static GSourceFuncs source_funcs = {
+  prepare,
+  check,
+  dispatch,
+  finalize
+};
+
+static void
+test_maincontext_source_finalization (void)
+{
+  GSource *source;
+
+  /* Check if GSource destruction without dispose function works and calls the
+   * finalize function as expected */
+  source_finalize_called = FALSE;
+  source_dispose_called = 0;
+  source_dispose_recycle = FALSE;
+  source = g_source_new (&source_funcs, sizeof (GSource));
+  g_source_unref (source);
+  g_assert_cmpint (source_dispose_called, ==, 0);
+  g_assert_true (source_finalize_called);
+
+  /* Check if GSource destruction with dispose function works and calls the
+   * dispose and finalize function as expected */
+  source_finalize_called = FALSE;
+  source_dispose_called = 0;
+  source_dispose_recycle = FALSE;
+  source = g_source_new (&source_funcs, sizeof (GSource));
+  g_source_set_dispose_function (source, dispose);
+  g_source_unref (source);
+  g_assert_cmpint (source_dispose_called, ==, 1);
+  g_assert_true (source_finalize_called);
+
+  /* Check if GSource destruction with dispose function works and recycling
+   * the source from dispose works without calling the finalize function */
+  source_finalize_called = FALSE;
+  source_dispose_called = 0;
+  source_dispose_recycle = TRUE;
+  source = g_source_new (&source_funcs, sizeof (GSource));
+  g_source_set_dispose_function (source, dispose);
+  g_source_unref (source);
+  g_assert_cmpint (source_dispose_called, ==, 1);
+  g_assert_false (source_finalize_called);
+
+  /* Check if the source is properly recycled */
+  g_assert_cmpint (source->ref_count, ==, 1);
+
+  /* And then get rid of it properly */
+  source_dispose_recycle = FALSE;
+  g_source_unref (source);
+  g_assert_cmpint (source_dispose_called, ==, 2);
+  g_assert_true (source_finalize_called);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -1753,6 +1833,7 @@ main (int argc, char *argv[])
   g_test_bug_base ("http://bugzilla.gnome.org/");
 
   g_test_add_func ("/maincontext/basic", test_maincontext_basic);
+  g_test_add_func ("/maincontext/source_finalization", test_maincontext_source_finalization);
   g_test_add_func ("/mainloop/basic", test_mainloop_basic);
   g_test_add_func ("/mainloop/timeouts", test_timeouts);
   g_test_add_func ("/mainloop/priorities", test_priorities);