From eb8a33625e6d79722d457cbcd84fd35e1dc826a4 Mon Sep 17 00:00:00 2001 From: Christian Hergert Date: Fri, 25 Aug 2023 14:20:27 -0700 Subject: [PATCH] gobject: Separate GWeakRef from GWeakNotify 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. # Conflicts: # gobject/tests/signals.c --- gobject/gobject.c | 18 ++++++++++++++---- gobject/tests/signals.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index c6fef8282..a9119bcca 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -260,6 +260,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 @@ -523,6 +524,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"); @@ -1359,9 +1361,16 @@ 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. Clear those first + * so that user code can rely on the state of their weak pointers. + */ 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 @@ -3316,7 +3325,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++; @@ -3331,7 +3340,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); } @@ -3355,7 +3364,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; @@ -3927,6 +3936,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 92a288215..a821606e0 100644 --- a/gobject/tests/signals.c +++ b/gobject/tests/signals.c @@ -2045,6 +2045,44 @@ test_emitv (void) g_array_unref (values); } +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 @@ -2083,6 +2121,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 (); }