Merge branch 'cancellable-connection-fixes' into 'main'

cancellable: Fix connect and disconnect references handling

Closes #3642 and #3643

See merge request GNOME/glib!4573

gio/gcancellable.c

@@ -264,6 +264,11 @@ g_cancellable_get_current  (void)
  * is to drop the reference to a cancellable after cancelling it,
  * and let it die with the outstanding async operations. You should
  * create a fresh cancellable for further async operations.
+ *
+ * In the event that a [signal@Gio.Cancellable::cancelled] signal handler is currently
+ * running, this call will block until the handler has finished.
+ * Calling this function from a signal handler will therefore result in a
+ * deadlock.
  **/
 void 
 g_cancellable_reset (GCancellable *cancellable)
@@ -392,6 +397,11 @@ g_cancellable_get_fd (GCancellable *cancellable)
  * readable status. Reading to unset the readable status is done
  * with g_cancellable_reset().
  *
+ * Note that in the event that a [signal@Gio.Cancellable::cancelled] signal handler is
+ * currently running, this call will block until the handler has finished.
+ * Calling this function from a signal handler will therefore result in a
+ * deadlock.
+ *
  * Returns: %TRUE if @pollfd was successfully initialized, %FALSE on 
  *          failure to prepare the cancellable.
  * 
@@ -440,7 +450,12 @@ g_cancellable_make_pollfd (GCancellable *cancellable, GPollFD *pollfd)
  * block scarce file descriptors until it is finalized if this function
  * is not called. This can cause the application to run out of file 
  * descriptors when many #GCancellables are used at the same time.
- * 
+ *
+ * Note that in the event that a [signal@Gio.Cancellable::cancelled] signal handler is
+ * currently running, this call will block until the handler has finished.
+ * Calling this function from a signal handler will therefore result in a
+ * deadlock.
+ *
  * Since: 2.22
  **/
 void
@@ -484,6 +499,9 @@ g_cancellable_release_fd (GCancellable *cancellable)
  * cancel the operation from the same thread in which it is running,
  * then the operation's #GAsyncReadyCallback will not be invoked until
  * the application returns to the main loop.
+ *
+ * It is safe (although useless, since it will be a no-op) to call
+ * this function from a [signal@Gio.Cancellable::cancelled] signal handler.
  **/
 void
 g_cancellable_cancel (GCancellable *cancellable)
@@ -513,6 +531,13 @@ g_cancellable_cancel (GCancellable *cancellable)
   if (priv->wakeup)
     GLIB_PRIVATE_CALL (g_wakeup_signal) (priv->wakeup);
 
+  /* Adding another reference, in case the callback is unreffing the
+   * cancellable and there are toggle references, so that the second to last
+   * reference (that would lead a toggle notification) won't be released
+   * while we're locked.
+   */
+  g_object_ref (cancellable);
+
   g_signal_emit (cancellable, signals[CANCELLED], 0);
 
   if (g_atomic_int_dec_and_test (&priv->cancelled_running))
@@ -521,6 +546,7 @@ g_cancellable_cancel (GCancellable *cancellable)
   g_mutex_unlock (&priv->mutex);
 
   g_object_unref (cancellable);
+  g_object_unref (cancellable);
 }
 
 /**
@@ -538,7 +564,10 @@ g_cancellable_cancel (GCancellable *cancellable)
  * either directly at the time of the connect if @cancellable is already
  * cancelled, or when @cancellable is cancelled in some thread.
  * In case the cancellable is reset via [method@Gio.Cancellable.reset]
- * then the callback can be called again if the @cancellable is cancelled.
+ * then the callback can be called again if the @cancellable is cancelled and
+ * if it had not been previously cancelled at the time
+ * [method@Gio.Cancellable.connect] was called (e.g. if the connection actually
+ * took place, returning a non-zero value).
  *
  * @data_destroy_func will be called when the handler is
  * disconnected, or immediately if the cancellable is already
@@ -547,9 +576,21 @@ g_cancellable_cancel (GCancellable *cancellable)
  * See #GCancellable::cancelled for details on how to use this.
  *
  * Since GLib 2.40, the lock protecting @cancellable is not held when
- * @callback is invoked.  This lifts a restriction in place for
+ * @callback is invoked. This lifts a restriction in place for
  * earlier GLib versions which now makes it easier to write cleanup
- * code that unconditionally invokes e.g. g_cancellable_cancel().
+ * code that unconditionally invokes e.g. [method@Gio.Cancellable.cancel].
+ * Note that since 2.82 GLib still holds a lock during the callback but it’s
+ * designed in a way that most of the [class@Gio.Cancellable] methods can be
+ * called, including [method@Gio.Cancellable.cancel] or
+ * [method@GObject.Object.unref].
+ *
+ * There are still some methods that will deadlock (by design) when
+ * called from the [signal@Gio.Cancellable::cancelled] callbacks:
+ *  - [method@Gio.Cancellable.connect]
+ *  - [method@Gio.Cancellable.disconnect]
+ *  - [method@Gio.Cancellable.reset]
+ *  - [method@Gio.Cancellable.make_pollfd]
+ *  - [method@Gio.Cancellable.release_fd]
  *
  * Returns: The id of the signal handler or 0 if @cancellable has already
  *          been cancelled.
@@ -562,10 +603,21 @@ g_cancellable_connect (GCancellable   *cancellable,
 		       gpointer        data,
 		       GDestroyNotify  data_destroy_func)
 {
+  GCancellable *extra_ref = NULL;
   gulong id;
 
   g_return_val_if_fail (G_IS_CANCELLABLE (cancellable), 0);
 
+  /* If the cancellable is already cancelled we may end up calling the callback
+   * immediately, and the callback may unref the Cancellable, so we need to add
+   * an extra reference here. We can't do it only in the case the cancellable
+   * is already cancelled because it can be potentially be reset, so we can't
+   * rely on the atomic value only, but we need to be locked to be really sure.
+   * At the same time we don't want to wake up the ToggleNotify if toggle
+   * references are enabled while we're locked.
+   */
+  g_object_ref (cancellable);
+
   g_mutex_lock (&cancellable->priv->mutex);
 
   if (g_atomic_int_get (&cancellable->priv->cancelled))
@@ -573,24 +625,37 @@ g_cancellable_connect (GCancellable   *cancellable,
       void (*_callback) (GCancellable *cancellable,
                          gpointer      user_data);
 
+      /* Adding another reference, in case the callback is unreffing the
+       * cancellable and there are toggle references, so that the second to last
+       * reference (that would lead a toggle notification) won't be released
+       * while we're locked.
+       */
+      extra_ref = g_object_ref (cancellable);
+
       _callback = (void *)callback;
       id = 0;
 
       _callback (cancellable, data);
-
-      if (data_destroy_func)
-        data_destroy_func (data);
     }
   else
     {
-      id = g_signal_connect_data (cancellable, "cancelled",
-                                  callback, data,
-                                  (GClosureNotify) data_destroy_func,
-                                  G_CONNECT_DEFAULT);
+      GClosure *closure;
+
+      closure = g_cclosure_new (callback, g_steal_pointer (&data),
+                                (GClosureNotify) g_steal_pointer (&data_destroy_func));
+
+      id = g_signal_connect_closure_by_id (cancellable, signals[CANCELLED],
+                                           0, closure, FALSE);
     }
 
   g_mutex_unlock (&cancellable->priv->mutex);
 
+  if (data_destroy_func)
+    data_destroy_func (data);
+
+  g_object_unref (cancellable);
+  g_clear_object (&extra_ref);
+
   return id;
 }
 

gio/tests/cancellable.c

@@ -712,6 +712,9 @@ on_racy_cancellable_cancelled (GCancellable *cancellable,
 {
   gboolean *callback_called = data;  /* (atomic) */
 
+  /* This must be a no-op and never dead-lock here! */
+  g_cancellable_cancel (cancellable);
+
   g_assert_true (g_cancellable_is_cancelled (cancellable));
   g_atomic_int_set (callback_called, TRUE);
 }
@@ -875,6 +878,293 @@ test_cancellable_source_can_be_fired_multiple_times (void)
   g_object_unref (cancellable);
 }
 
+static void
+data_cleanup_cb (gpointer user_data)
+{
+  gboolean *data_cleanup_called = user_data;
+
+  *data_cleanup_called = TRUE;
+}
+
+static void
+test_connect_data_is_destroyed_on_disconnect_and_dispose (void)
+{
+  GCancellable *cancellable;
+  gboolean data_cleanup_called;
+  gulong id;
+
+  cancellable = g_cancellable_new ();
+
+  data_cleanup_called = FALSE;
+  id = g_cancellable_connect (cancellable, G_CALLBACK (do_nothing),
+                              &data_cleanup_called, data_cleanup_cb);
+  g_assert_cmpuint (id, >, 0);
+  g_cancellable_disconnect (cancellable, id);
+  g_assert_true (data_cleanup_called);
+
+  data_cleanup_called = FALSE;
+  id = g_cancellable_connect (cancellable, G_CALLBACK (do_nothing),
+                              &data_cleanup_called, data_cleanup_cb);
+  g_assert_cmpuint (id, >, 0);
+  g_clear_object (&cancellable);
+  g_assert_true (data_cleanup_called);
+}
+
+static void
+test_connect_cancelled_data_is_destroyed (void)
+{
+  GCancellable *cancellable;
+  gboolean data_cleanup_called;
+  gulong id;
+
+  cancellable = g_cancellable_new ();
+  data_cleanup_called = FALSE;
+  g_cancellable_cancel (cancellable);
+  id = g_cancellable_connect (cancellable, G_CALLBACK (do_nothing),
+                              &data_cleanup_called, data_cleanup_cb);
+  g_assert_cmpuint (id, ==, 0);
+  g_assert_true (data_cleanup_called);
+  g_clear_object (&cancellable);
+}
+
+static void
+assert_references_and_unref (GCancellable *cancellable, gpointer data)
+{
+  gint expected_references = GPOINTER_TO_INT (data);
+
+  /* This must be a no-op and never dead-lock here! */
+  g_cancellable_cancel (cancellable);
+
+  g_assert_cmpint (G_OBJECT (cancellable)->ref_count, ==, expected_references);
+  g_object_unref (cancellable);
+}
+
+static void
+test_connect_to_disposing_callback (void)
+{
+  GCancellable *cancellable;
+  gulong id;
+
+  g_test_summary ("A cancellable signal callback can unref the cancellable");
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/3643");
+
+  cancellable = g_cancellable_new ();
+  g_object_add_weak_pointer (G_OBJECT (cancellable), (gpointer *) &cancellable);
+
+  id = g_cancellable_connect (cancellable, G_CALLBACK (assert_references_and_unref),
+                              GINT_TO_POINTER (4), NULL);
+  g_assert_cmpuint (id, >, 0);
+  g_cancellable_cancel (cancellable);
+  g_assert_null (cancellable);
+}
+
+typedef struct
+{
+  gulong id;
+  gboolean ignore_next_toggle_down;
+} ToggleReferenceData;
+
+static void
+toggle_reference_cb (gpointer data,
+                     GObject *object,
+                     gboolean is_last_ref)
+{
+  GCancellable *cancellable = G_CANCELLABLE (object);
+  ToggleReferenceData *toggle_data = data;
+
+  g_test_message ("Toggle reference callback for %s (%p), last: %d",
+                  g_type_name_from_instance ((GTypeInstance *) object),
+                  object, is_last_ref);
+
+  if (!is_last_ref)
+    {
+      g_assert_false (g_cancellable_is_cancelled (cancellable));
+
+      /* Disconnect and reconnect to the signal so that we can verify that the
+       * "toggle-up" does not happen while we're locked
+       */
+      g_cancellable_disconnect (cancellable, toggle_data->id);
+      toggle_data->id = g_cancellable_connect (cancellable,
+                                               G_CALLBACK (assert_references_and_unref),
+                                               GINT_TO_POINTER (4), NULL);
+      return;
+    }
+
+  g_assert_true (is_last_ref);
+
+  if (toggle_data->ignore_next_toggle_down)
+    {
+      g_assert_false (g_cancellable_is_cancelled (cancellable));
+      toggle_data->ignore_next_toggle_down = FALSE;
+      return;
+    }
+
+  g_assert_true (g_cancellable_is_cancelled (cancellable));
+
+  /* This would deadlock if the last reference was removed during cancellation */
+  g_cancellable_disconnect (cancellable, toggle_data->id);
+  toggle_data->id = 0;
+}
+
+static void
+test_connect_to_disposing_callback_with_toggle_reference (void)
+{
+  GCancellable *cancellable;
+  ToggleReferenceData data = {0};
+
+  cancellable = g_cancellable_new ();
+  g_object_add_weak_pointer (G_OBJECT (cancellable), (gpointer *) &cancellable);
+
+  data.id = g_cancellable_connect (cancellable, G_CALLBACK (assert_references_and_unref),
+                                   GINT_TO_POINTER (4), NULL);
+
+  /* Switch to toggle references. */
+  g_object_add_toggle_ref (G_OBJECT (cancellable), toggle_reference_cb, &data);
+  data.ignore_next_toggle_down = TRUE;
+  g_object_unref (cancellable);
+
+  g_cancellable_cancel (cancellable);
+  g_assert_cmpuint (data.id, ==, 0);
+  g_assert_null (cancellable);
+}
+
+static void
+cancelled_toggle_reference_cb (gpointer data,
+                               GObject *object,
+                               gboolean is_last_ref)
+{
+  GCancellable *cancellable = G_CANCELLABLE (object);
+  ToggleReferenceData *toggle_data = data;
+
+  g_test_message ("Toggle reference callback for %s (%p), last: %d",
+                  g_type_name_from_instance ((GTypeInstance *) object),
+                  object, is_last_ref);
+
+  if (!is_last_ref)
+    {
+      gulong id;
+
+      if (g_cancellable_is_cancelled (cancellable))
+        {
+          /* Disconnect and reconnect to the signal so that we can verify that the
+           * "toggle-up" does not happen while we're locked
+           */
+          g_cancellable_disconnect (cancellable, toggle_data->id);
+          id = g_cancellable_connect (cancellable, G_CALLBACK (do_nothing), NULL, NULL);
+          g_assert_cmpuint (id, ==, 0);
+          return;
+        }
+
+      /* Connect and disconnect to the signal so that we can verify that the
+       * "toggle-up" does not happen while we're locked
+       */
+      id = g_cancellable_connect (cancellable, G_CALLBACK (do_nothing), NULL, NULL);
+      g_cancellable_disconnect (cancellable, id);
+      return;
+    }
+
+  g_assert_true (is_last_ref);
+
+  if (toggle_data->ignore_next_toggle_down)
+    {
+      g_assert_false (g_cancellable_is_cancelled (cancellable));
+      toggle_data->ignore_next_toggle_down = FALSE;
+      return;
+    }
+
+  g_assert_true (g_cancellable_is_cancelled (cancellable));
+
+  g_test_expect_message ("GLib-GObject", G_LOG_LEVEL_CRITICAL,
+                         "*has no handler with id*");
+
+  /* We try resetting the a signal that isn't connected, since we don't care
+   * about anything but checking wether this would deadlock
+   */
+  g_cancellable_disconnect (cancellable, G_MAXULONG);
+
+  g_test_assert_expected_messages ();
+}
+
+static void
+test_connect_cancelled_to_disposing_callback_with_toggle_reference (void)
+{
+  GCancellable *cancellable;
+  ToggleReferenceData data = {0};
+  gulong id;
+
+  cancellable = g_cancellable_new ();
+  g_object_add_weak_pointer (G_OBJECT (cancellable), (gpointer *) &cancellable);
+
+  /* Switch to toggle references. */
+  g_object_add_toggle_ref (G_OBJECT (cancellable), cancelled_toggle_reference_cb, &data);
+  data.ignore_next_toggle_down = TRUE;
+  g_object_unref (cancellable);
+
+  g_cancellable_cancel (cancellable);
+  id = g_cancellable_connect (cancellable, G_CALLBACK (assert_references_and_unref),
+                              GINT_TO_POINTER (3), NULL);
+
+  g_assert_cmpuint (id, ==, 0);
+  g_assert_null (cancellable);
+}
+
+static void
+test_connect_cancelled_to_disposing_callback (void)
+{
+  GCancellable *cancellable;
+  gulong id;
+
+  g_test_summary ("A cancellable signal callback can unref the cancellable");
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/3643");
+
+  cancellable = g_cancellable_new ();
+  g_object_add_weak_pointer (G_OBJECT (cancellable), (gpointer *) &cancellable);
+
+  g_cancellable_cancel (cancellable);
+  id = g_cancellable_connect (cancellable, G_CALLBACK (assert_references_and_unref),
+                              GINT_TO_POINTER (3), NULL);
+  g_assert_cmpuint (id, ==, 0);
+  g_assert_null (cancellable);
+}
+
+static void
+on_connect_data_callback (GCancellable *cancellable,
+                          gpointer user_data)
+{
+  g_assert_true (cancellable == user_data);
+}
+
+static void
+connect_data_destroy (gpointer user_data)
+{
+  GCancellable *cancellable = user_data;
+
+  g_assert_true (g_cancellable_is_cancelled (cancellable));
+
+  /* We try resetting the cancellable, since we don't care
+   * about anything but checking wether this would deadlock
+   */
+  g_cancellable_reset (cancellable);
+  g_object_unref (cancellable);
+}
+
+static void
+test_connect_cancelled_with_destroy_func_disposing_cancellable (void)
+{
+  GCancellable *cancellable;
+  gulong id;
+
+  cancellable = g_cancellable_new ();
+  g_object_add_weak_pointer (G_OBJECT (cancellable), (gpointer *) &cancellable);
+
+  g_cancellable_cancel (cancellable);
+  id = g_cancellable_connect (cancellable, G_CALLBACK (on_connect_data_callback),
+                              cancellable, connect_data_destroy);
+  g_assert_cmpuint (id, ==, 0);
+
+  g_assert_null (cancellable);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -882,6 +1172,13 @@ main (int argc, char *argv[])
 
   g_test_add_func ("/cancellable/multiple-concurrent", test_cancel_multiple_concurrent);
   g_test_add_func ("/cancellable/null", test_cancel_null);
+  g_test_add_func ("/cancellable/connect-data-is-destroyed-on-disconnect-and-dispose", test_connect_data_is_destroyed_on_disconnect_and_dispose);
+  g_test_add_func ("/cancellable/connect-to-disposing-callback", test_connect_to_disposing_callback);
+  g_test_add_func ("/cancellable/connect-cancelled-data-is-destroyed", test_connect_cancelled_data_is_destroyed);
+  g_test_add_func ("/cancellable/connect-to-disposing-callback-with-toggle-reference", test_connect_to_disposing_callback_with_toggle_reference);
+  g_test_add_func ("/cancellable/connect-cancelled-to-disposing-callback", test_connect_cancelled_to_disposing_callback);
+  g_test_add_func ("/cancellable/connect-cancelled-with-destroy-func-disposing-cancellable", test_connect_cancelled_with_destroy_func_disposing_cancellable);
+  g_test_add_func ("/cancellable/connect-cancelled-to-disposing-callback-with-toggle-reference", test_connect_cancelled_to_disposing_callback_with_toggle_reference);
   g_test_add_func ("/cancellable/disconnect-on-cancelled-callback-hangs", test_cancellable_disconnect_on_cancelled_callback_hangs);
   g_test_add_func ("/cancellable/resets-on-cancel-callback-hangs", test_cancellable_reset_on_cancelled_callback_hangs);
   g_test_add_func ("/cancellable/poll-fd", test_cancellable_poll_fd);