gobject: Separate GWeakRef from GWeakNotifywip/chergert/separate-gweakref-from-gweaknotify-gdata

This patch is based upon Garrett Regier's work from 2015 to provide
some reliability and predictability to how disposal handles weak
reference state.

A primary problem is that GWeakRef and GWeakNotify state is shared and
therefore you cannot rely on GWeakRef status due to a GWeakNotify
calling into second-degree code.

It's important to ensure that both weak pointer locations and GWeakRef
will do the proper thing before user callbacks are executed during
disposal. Otherwise, user callbacks cannot rely on the status of their
weak pointers. That would be mostly okay but becomes an issue when
second degree objects are then disposed before any notification of
dependent object disposal.

Consider objects A and B.

`A` contains a reference to `B` and `B` contains a `GWeakRef` to `A`.
When `A` is disposed, `B` may be disposed as a consequence but has not
yet been notified that `A` has been disposed. It's `GWeakRef` may also
cause liveness issues if `GWeakNotify` on `A` result in tertiary code
running which wants to interact with `B`.

This example is analagous to how `GtkTextView` and `GtkTextBuffer` work
in text editing applications.

To provide application and libraries the ability to handle this using
already existing API, `GWeakRef` is separated into it's own GData quark
so that weak locations and `GWeakRef` are cleared before user code is
executed as a consequence of `GData` cleanup.

2 files changed, 51 insertions, 4 deletions

diff --git a/gobject/gobject.c b/gobject/gobject.c
index a61754b9f..6b0ea6616 100644
--- a/gobject/gobject.c
+++ b/gobject/gobject.c
@@ -261,6 +261,7 @@ G_LOCK_DEFINE_STATIC (weak_refs_mutex);
 G_LOCK_DEFINE_STATIC (toggle_refs_mutex);
 static GQuark	            quark_closure_array = 0;
 static GQuark	            quark_weak_refs = 0;
+static GQuark	            quark_weak_notifies = 0;
 static GQuark	            quark_toggle_refs = 0;
 static GQuark               quark_notify_queue;
 #ifndef HAVE_OPTIONAL_FLAGS
@@ -524,6 +525,7 @@ g_object_do_class_init (GObjectClass *class)
   quark_closure_array = g_quark_from_static_string ("GObject-closure-array");
 
   quark_weak_refs = g_quark_from_static_string ("GObject-weak-references");
+  quark_weak_notifies = g_quark_from_static_string ("GObject-weak-notifies");
   quark_weak_locations = g_quark_from_static_string ("GObject-weak-locations");
   quark_toggle_refs = g_quark_from_static_string ("GObject-toggle-references");
   quark_notify_queue = g_quark_from_static_string ("GObject-notify-queue");
@@ -1360,9 +1362,14 @@ static void
 g_object_real_dispose (GObject *object)
 {
   g_signal_handlers_destroy (object);
-  g_datalist_id_set_data (&object->qdata, quark_closure_array, NULL);
+
+  /* GWeakRef and weak_pointer do not call into user code */
   g_datalist_id_set_data (&object->qdata, quark_weak_refs, NULL);
   g_datalist_id_set_data (&object->qdata, quark_weak_locations, NULL);
+
+  /* GWeakNotify and GClosure can call into user code */
+  g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
+  g_datalist_id_set_data (&object->qdata, quark_closure_array, NULL);
 }
 
 #ifdef G_ENABLE_DEBUG
@@ -3317,7 +3324,7 @@ g_object_weak_ref (GObject    *object,
   g_return_if_fail (g_atomic_int_get (&object->ref_count) >= 1);
 
   G_LOCK (weak_refs_mutex);
-  wstack = g_datalist_id_remove_no_notify (&object->qdata, quark_weak_refs);
+  wstack = g_datalist_id_remove_no_notify (&object->qdata, quark_weak_notifies);
   if (wstack)
     {
       i = wstack->n_weak_refs++;
@@ -3332,7 +3339,7 @@ g_object_weak_ref (GObject    *object,
     }
   wstack->weak_refs[i].notify = notify;
   wstack->weak_refs[i].data = data;
-  g_datalist_id_set_data_full (&object->qdata, quark_weak_refs, wstack, weak_refs_notify);
+  g_datalist_id_set_data_full (&object->qdata, quark_weak_notifies, wstack, weak_refs_notify);
   G_UNLOCK (weak_refs_mutex);
 }
 
@@ -3356,7 +3363,7 @@ g_object_weak_unref (GObject    *object,
   g_return_if_fail (notify != NULL);
 
   G_LOCK (weak_refs_mutex);
-  wstack = g_datalist_id_get_data (&object->qdata, quark_weak_refs);
+  wstack = g_datalist_id_get_data (&object->qdata, quark_weak_notifies);
   if (wstack)
     {
       guint i;
@@ -3924,6 +3931,7 @@ g_object_unref (gpointer _object)
       g_signal_handlers_destroy (object);
       g_datalist_id_set_data (&object->qdata, quark_weak_refs, NULL);
       g_datalist_id_set_data (&object->qdata, quark_weak_locations, NULL);
+      g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
 
       /* decrement the last reference */
       old_ref = g_atomic_int_add (&object->ref_count, -1);
diff --git a/gobject/tests/signals.c b/gobject/tests/signals.c
index 1c7085244..310b7f2e6 100644
--- a/gobject/tests/signals.c
+++ b/gobject/tests/signals.c
@@ -1788,6 +1788,44 @@ test_signal_is_valid_name (void)
     g_assert_false (g_signal_is_valid_name (invalid_names[i]));
 }
 
+typedef struct
+{
+  GWeakRef wr;
+  gulong handler;
+} TestWeakRefDisconnect;
+
+static void
+weak_ref_disconnect_notify (gpointer  data,
+                            GObject  *where_object_was)
+{
+  TestWeakRefDisconnect *state = data;
+  g_assert_null (g_weak_ref_get (&state->wr));
+  state->handler = 0;
+}
+
+static void
+test_weak_ref_disconnect (void)
+{
+  TestWeakRefDisconnect state;
+  GObject *test;
+
+  test = g_object_new (test_get_type (), NULL);
+  g_weak_ref_init (&state.wr, test);
+  state.handler = g_signal_connect_data (test,
+                                         "simple",
+                                         G_CALLBACK (dont_reach),
+                                         &state,
+                                         (GClosureNotify) weak_ref_disconnect_notify,
+                                         0);
+  g_assert_cmpint (state.handler, >, 0);
+
+  g_object_unref (test);
+
+  g_assert_cmpint (state.handler, ==, 0);
+  g_assert_null (g_weak_ref_get (&state.wr));
+  g_weak_ref_clear (&state.wr);
+}
+
 /* --- */
 
 int
@@ -1825,6 +1863,7 @@ main (int argc,
   g_test_add_data_func ("/gobject/signals/invalid-name/first-char", "7zip", test_signals_invalid_name);
   g_test_add_data_func ("/gobject/signals/invalid-name/empty", "", test_signals_invalid_name);
   g_test_add_func ("/gobject/signals/is-valid-name", test_signal_is_valid_name);
+  g_test_add_func ("/gobject/signals/weak-ref-disconnect", test_weak_ref_disconnect);
 
   return g_test_run ();
 }