[glib: 1/2] gobject: Ensure an object has toggle references before notifying it

[Philip Withnall <pwithnall src gnome org>]

commit 468246bb3b83539bcb83397a7cec6d767573bfb2
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Wed Apr 28 21:54:45 2021 +0200

    gobject: Ensure an object has toggle references before notifying it
    
    When an object with toggle reference is notifying a change we just
    assume that this is true because of previous checks.
    However, while locking, another thread may have removed the toggle
    reference causing the waiting thread to abort (as no handler is set at
    that point).
    
    To avoid this, once we've got the toggle references mutex lock, check
    again if the object has toggle reference, and if it's not the case
    anymore just ignore the request.
    
    Add a test that triggers this, it's not 100% happening because this is
    of course timing related, but this is very close to the truth.
    
    Fixes: #2394

 gobject/gobject.c           | 10 ++++++
 gobject/tests/threadtests.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)
---
diff --git a/gobject/gobject.c b/gobject/gobject.c
index 2c62b5aae..9ddab43f0 100644
--- a/gobject/gobject.c
+++ b/gobject/gobject.c
@@ -3276,6 +3276,16 @@ toggle_refs_notify (GObject *object,
   ToggleRefStack tstack, *tstackptr;
 
   G_LOCK (toggle_refs_mutex);
+  /* If another thread removed the toggle reference on the object, while
+   * we were waiting here, there's nothing to notify.
+   * So let's check again if the object has toggle reference and in case return.
+   */
+  if (!OBJECT_HAS_TOGGLE_REF (object))
+    {
+      G_UNLOCK (toggle_refs_mutex);
+      return;
+    }
+
   tstackptr = g_datalist_id_get_data (&object->qdata, quark_toggle_refs);
   tstack = *tstackptr;
   G_UNLOCK (toggle_refs_mutex);
diff --git a/gobject/tests/threadtests.c b/gobject/tests/threadtests.c
index b09b79f9b..3b485eb52 100644
--- a/gobject/tests/threadtests.c
+++ b/gobject/tests/threadtests.c
@@ -422,6 +422,91 @@ test_threaded_weak_ref_finalization (void)
   g_assert_null (g_weak_ref_get (&weak));
 }
 
+typedef struct
+{
+  GObject *object;
+  int done;    /* (atomic) */
+  int toggles; /* (atomic) */
+} ToggleNotifyThreadData;
+
+static gpointer
+on_reffer_thread (gpointer user_data)
+{
+  ToggleNotifyThreadData *thread_data = user_data;
+
+  while (!g_atomic_int_get (&thread_data->done))
+    {
+      g_object_ref (thread_data->object);
+      g_object_unref (thread_data->object);
+    }
+
+  return NULL;
+}
+
+static void
+on_toggle_notify (gpointer data,
+                  GObject *object,
+                  gboolean is_last_ref)
+{
+  /* Anything could be put here, but we don't care for this test.
+   * Actually having this empty made the bug to happen more frequently (being
+   * timing related).
+   */
+}
+
+static gpointer
+on_toggler_thread (gpointer user_data)
+{
+  ToggleNotifyThreadData *thread_data = user_data;
+
+  while (!g_atomic_int_get (&thread_data->done))
+    {
+      g_object_ref (thread_data->object);
+      g_object_remove_toggle_ref (thread_data->object, on_toggle_notify, thread_data);
+      g_object_add_toggle_ref (thread_data->object, on_toggle_notify, thread_data);
+      g_object_unref (thread_data->object);
+      g_atomic_int_add (&thread_data->toggles, 1);
+    }
+
+  return NULL;
+}
+
+static void
+test_threaded_toggle_notify (void)
+{
+  GObject *object = g_object_new (G_TYPE_OBJECT, NULL);
+  ToggleNotifyThreadData data = { object, FALSE, 0 };
+  GThread *threads[3];
+  gsize i;
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/issues/2394";);
+  g_test_summary ("Test that toggle reference notifications can be changed "
+                  "safely from another (the main) thread without causing the "
+                  "notifying thread to abort");
+
+  g_object_add_toggle_ref (object, on_toggle_notify, &data);
+  g_object_unref (object);
+
+  g_assert_cmpint (object->ref_count, ==, 1);
+  threads[0] = g_thread_new ("on_reffer_thread", on_reffer_thread, &data);
+  threads[1] = g_thread_new ("on_another_reffer_thread", on_reffer_thread, &data);
+  threads[2] = g_thread_new ("on_main_toggler_thread", on_toggler_thread, &data);
+
+  /* We need to wait here for the threads to run for a bit in order to make the
+   * race to happen, so we wait for an high number of toggle changes to be met
+   * so that we can be consistent on each platform.
+   */
+  while (g_atomic_int_get (&data.toggles) < 1000000)
+    ;
+  g_atomic_int_set (&data.done, TRUE);
+
+  for (i = 0; i < G_N_ELEMENTS (threads); i++)
+    g_thread_join (threads[i]);
+
+  g_assert_cmpint (object->ref_count, ==, 1);
+  g_clear_object (&object);
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -433,6 +518,8 @@ main (int   argc,
   g_test_add_func ("/GObject/threaded-weak-ref", test_threaded_weak_ref);
   g_test_add_func ("/GObject/threaded-weak-ref/on-finalization",
                    test_threaded_weak_ref_finalization);
+  g_test_add_func ("/GObject/threaded-toggle-notify",
+                   test_threaded_toggle_notify);
 
   return g_test_run();
 }