From 61af8293159c95be7ca961a9a854ac039feaf4ce Mon Sep 17 00:00:00 2001 From: Christian Hergert Date: Wed, 8 Mar 2023 10:56:51 -0800 Subject: 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. --- gobject/gobject.c | 16 ++++++++++++---- gobject/tests/signals.c | 39 +++++++++++++++++++++++++++++++++++++++ 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 (); } -- cgit v1.2.1