[glib: 1/4] gobject: Cleanup GWeakRef locations on object finalization

[Philip Withnall <pwithnall src gnome org>]

commit ea68b22135f6ca41b9d520ab169ae2346e29d276
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Sat Apr 24 04:39:17 2021 +0200

    gobject: Cleanup GWeakRef locations on object finalization
    
    It can happen that a GWeakRef is added to an object while it's disposing
    (or even during finalizing) and this may happen in a thread that (weak)
    references an object while the disposal isn't completed yet or when
    using toggle references and switching to GWeakRef on notification (as
    the API suggests).
    
    In such scenario the weak locations are not cleaned up when the object
    is finalized, and will point to a free'd area.
    
    So, during finalization and when we're sure that the object will be
    destroyed for sure, check again if there are new weak locations and
    unset them if any as part of the qdata destruction.
    Do this adding a new utility function so that we can avoid duplicating
    code to free the weak locations.
    
    Added various tests simulating this case.
    
    Fixes: #2390

 gobject/gobject.c           |  46 ++++++++++++++++----
 gobject/tests/reference.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++
 gobject/tests/threadtests.c |  81 +++++++++++++++++++++++++++++++++++
 3 files changed, 221 insertions(+), 8 deletions(-)
---
diff --git a/gobject/gobject.c b/gobject/gobject.c
index 9776d7c95..c558c1197 100644
--- a/gobject/gobject.c
+++ b/gobject/gobject.c
@@ -233,6 +233,7 @@ static guint               object_floating_flag_handler (GObject        *object,
 
 static void object_interface_check_properties           (gpointer        check_data,
                                                         gpointer        g_iface);
+static void                weak_locations_free_unlocked (GSList **weak_locations);
 
 /* --- typedefs --- */
 typedef struct _GObjectNotifyQueue            GObjectNotifyQueue;
@@ -3512,6 +3513,10 @@ g_object_unref (gpointer _object)
        * established at this time, because the other thread would have
        * to hold a strong ref in order to call
        * g_object_add_weak_pointer() and then we wouldn't be here.
+       *
+       * Other GWeakRef's (weak locations) instead may still be added
+       * before the object is finalized, but in such case we'll unset
+       * them as part of the qdata removal.
        */
       weak_locations = g_datalist_id_get_data (&object->qdata, quark_weak_locations);
 
@@ -3533,13 +3538,8 @@ g_object_unref (gpointer _object)
           /* We got the lock first, so the object will definitely die
            * now. Clear out all the weak references.
            */
-          while (*weak_locations)
-            {
-              GWeakRef *weak_ref_location = (*weak_locations)->data;
-
-              weak_ref_location->priv.p = NULL;
-              *weak_locations = g_slist_delete_link (*weak_locations, *weak_locations);
-            }
+          weak_locations_free_unlocked (weak_locations);
+          g_datalist_id_remove_no_notify (&object->qdata, quark_weak_locations);
 
           g_rw_lock_writer_unlock (&weak_locations_lock);
         }
@@ -4646,6 +4646,35 @@ g_weak_ref_get (GWeakRef *weak_ref)
   return object_or_null;
 }
 
+static void
+weak_locations_free_unlocked (GSList **weak_locations)
+{
+  if (*weak_locations)
+    {
+      GSList *weak_location;
+
+      for (weak_location = *weak_locations; weak_location;)
+        {
+          GWeakRef *weak_ref_location = weak_location->data;
+
+          weak_ref_location->priv.p = NULL;
+          weak_location = g_slist_delete_link (weak_location, weak_location);
+        }
+    }
+
+  g_free (weak_locations);
+}
+
+static void
+weak_locations_free (gpointer data)
+{
+  GSList **weak_locations = data;
+
+  g_rw_lock_writer_lock (&weak_locations_lock);
+  weak_locations_free_unlocked (weak_locations);
+  g_rw_lock_writer_unlock (&weak_locations_lock);
+}
+
 /**
  * g_weak_ref_set: (skip)
  * @weak_ref: location for a weak reference
@@ -4712,7 +4741,8 @@ g_weak_ref_set (GWeakRef *weak_ref,
           if (weak_locations == NULL)
             {
               weak_locations = g_new0 (GSList *, 1);
-              g_datalist_id_set_data_full (&new_object->qdata, quark_weak_locations, weak_locations, g_free);
+              g_datalist_id_set_data_full (&new_object->qdata, quark_weak_locations,
+                                           weak_locations, weak_locations_free);
             }
 
           *weak_locations = g_slist_prepend (*weak_locations, weak_ref);
diff --git a/gobject/tests/reference.c b/gobject/tests/reference.c
index 04645e14a..de49ff1fd 100644
--- a/gobject/tests/reference.c
+++ b/gobject/tests/reference.c
@@ -566,6 +566,106 @@ test_weak_ref (void)
   g_free (dynamic_weak);
 }
 
+G_DECLARE_FINAL_TYPE (WeakReffedObject, weak_reffed_object,
+                      WEAK, REFFED_OBJECT, GObject)
+
+struct _WeakReffedObject
+{
+  GObject parent;
+
+  GWeakRef *weak_ref;
+};
+
+G_DEFINE_TYPE (WeakReffedObject, weak_reffed_object, G_TYPE_OBJECT)
+
+static void
+weak_reffed_object_dispose (GObject *object)
+{
+  WeakReffedObject *weak_reffed = WEAK_REFFED_OBJECT (object);
+
+  g_assert_cmpint (object->ref_count, ==, 1);
+
+  g_weak_ref_set (weak_reffed->weak_ref, object);
+
+  G_OBJECT_CLASS (weak_reffed_object_parent_class)->dispose (object);
+}
+
+static void
+weak_reffed_object_init (WeakReffedObject *connector)
+{
+}
+
+static void
+weak_reffed_object_class_init (WeakReffedObjectClass *klass)
+{
+  GObjectClass *object_class = G_OBJECT_CLASS (klass);
+
+  object_class->dispose = weak_reffed_object_dispose;
+}
+
+static void
+test_weak_ref_on_dispose (void)
+{
+  WeakReffedObject *obj;
+  GWeakRef weak = { { GUINT_TO_POINTER (0xDEADBEEFU) } };
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2390";);
+  g_test_summary ("Test that a weak ref set during dispose vfunc is cleared");
+
+  g_weak_ref_init (&weak, NULL);
+
+  obj = g_object_new (weak_reffed_object_get_type (), NULL);
+  obj->weak_ref = &weak;
+
+  g_assert_cmpint (G_OBJECT (obj)->ref_count, ==, 1);
+  g_clear_object (&obj);
+
+  g_assert_null (g_weak_ref_get (&weak));
+}
+
+static void
+on_weak_ref_toggle_notify (gpointer data,
+                           GObject *object,
+                           gboolean is_last_ref)
+{
+  GWeakRef *weak = data;
+
+  if (is_last_ref)
+    g_weak_ref_set (weak, object);
+}
+
+static void
+on_weak_ref_toggle_notify_disposed (gpointer data,
+                                    GObject *object)
+{
+  g_assert_cmpint (object->ref_count, ==, 1);
+
+  g_object_ref (object);
+  g_object_unref (object);
+}
+
+static void
+test_weak_ref_on_toggle_notify (void)
+{
+  GObject *obj;
+  GWeakRef weak = { { GUINT_TO_POINTER (0xDEADBEEFU) } };
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2390";);
+  g_test_summary ("Test that a weak ref set on toggle notify is cleared");
+
+  g_weak_ref_init (&weak, NULL);
+
+  obj = g_object_new (G_TYPE_OBJECT, NULL);
+  g_object_add_toggle_ref (obj, on_weak_ref_toggle_notify, &weak);
+  g_object_weak_ref (obj, on_weak_ref_toggle_notify_disposed, NULL);
+  g_object_unref (obj);
+
+  g_assert_cmpint (obj->ref_count, ==, 1);
+  g_clear_object (&obj);
+
+  g_assert_null (g_weak_ref_get (&weak));
+}
+
 typedef struct
 {
   gboolean should_be_last;
@@ -826,6 +926,8 @@ main (int argc, char **argv)
   g_test_add_func ("/object/weak-pointer/set", test_weak_pointer_set);
   g_test_add_func ("/object/weak-pointer/set-function", test_weak_pointer_set_function);
   g_test_add_func ("/object/weak-ref", test_weak_ref);
+  g_test_add_func ("/object/weak-ref/on-dispose", test_weak_ref_on_dispose);
+  g_test_add_func ("/object/weak-ref/on-toggle-notify", test_weak_ref_on_toggle_notify);
   g_test_add_func ("/object/toggle-ref", test_toggle_ref);
   g_test_add_func ("/object/qdata", test_object_qdata);
   g_test_add_func ("/object/qdata2", test_object_qdata2);
diff --git a/gobject/tests/threadtests.c b/gobject/tests/threadtests.c
index b6f9e17fa..b09b79f9b 100644
--- a/gobject/tests/threadtests.c
+++ b/gobject/tests/threadtests.c
@@ -343,6 +343,85 @@ test_threaded_weak_ref (void)
              get_wins, unref_wins);
 }
 
+typedef struct
+{
+  GObject *object;
+  GWeakRef *weak;
+  gint started; /* (atomic) */
+  gint finished; /* (atomic) */
+  gint disposing; /* (atomic) */
+} ThreadedWeakRefData;
+
+static void
+on_weak_ref_disposed (gpointer data,
+                      GObject *gobj)
+{
+  ThreadedWeakRefData *thread_data = data;
+
+  /* Wait until the thread has started */
+  while (!g_atomic_int_get (&thread_data->started))
+    continue;
+
+  g_atomic_int_set (&thread_data->disposing, 1);
+
+  /* Wait for the thread to act, so that the object is still valid */
+  while (!g_atomic_int_get (&thread_data->finished))
+    continue;
+
+  g_atomic_int_set (&thread_data->disposing, 0);
+}
+
+static gpointer
+on_other_thread_weak_ref (gpointer user_data)
+{
+  ThreadedWeakRefData *thread_data = user_data;
+  GObject *object = thread_data->object;
+
+  g_atomic_int_set (&thread_data->started, 1);
+
+  /* Ensure we've started disposal */
+  while (!g_atomic_int_get (&thread_data->disposing))
+    continue;
+
+  g_object_ref (object);
+  g_weak_ref_set (thread_data->weak, object);
+  g_object_unref (object);
+
+  g_assert_cmpint (thread_data->disposing, ==, 1);
+  g_atomic_int_set (&thread_data->finished, 1);
+
+  return NULL;
+}
+
+static void
+test_threaded_weak_ref_finalization (void)
+{
+  GObject *obj = g_object_new (G_TYPE_OBJECT, NULL);
+  GWeakRef weak = { { GUINT_TO_POINTER (0xDEADBEEFU) } };
+  ThreadedWeakRefData thread_data = {
+    .object = obj, .weak = &weak, .started = 0, .finished = 0
+  };
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2390";);
+  g_test_summary ("Test that a weak ref added by another thread during dispose "
+                  "of a GObject is cleared during finalisation. "
+                  "Use on_weak_ref_disposed() to synchronize the other thread "
+                  "with the dispose vfunc.");
+
+  g_weak_ref_init (&weak, NULL);
+  g_object_weak_ref (obj, on_weak_ref_disposed, &thread_data);
+
+  g_assert_cmpint (obj->ref_count, ==, 1);
+  g_thread_unref (g_thread_new ("on_other_thread",
+                                on_other_thread_weak_ref,
+                                &thread_data));
+  g_object_unref (obj);
+
+  /* This is what this test is about: at this point the weak reference
+   * should have been unset (and not point to a dead object either). */
+  g_assert_null (g_weak_ref_get (&weak));
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -352,6 +431,8 @@ main (int   argc,
   /* g_test_add_func ("/GObject/threaded-class-init", test_threaded_class_init); */
   g_test_add_func ("/GObject/threaded-object-init", test_threaded_object_init);
   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);
 
   return g_test_run();
 }