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

@@ -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)
+          weak_locations_free_unlocked (weak_locations);
-            {
+          g_datalist_id_remove_no_notify (&object->qdata, quark_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);
-            }
 
           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);

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);

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();
 }