From dadb759c652a8dfa9a86a11598fa05c00d273cd0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Apr 2025 11:00:49 +0200 Subject: [PATCH] gobject: invoke g_object_weak_ref() one-by-one during destruction Previously, at two places (in g_object_real_dispose() and shortly before finalize()), we would call g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL); This clears @quark_weak_notifies at once and then invokes all notifications. This means, if you were inside a notification callback and called g_object_weak_unref() on the object for *another* weak-reference, then an exception failed: GLib-GObject-FATAL-CRITICAL: g_object_weak_unref_cb: couldn't find weak ref 0x401320(0x16b9fe0) Granted, maybe inside a GWeakNotify you shouldn't call much of anything on where_the_object_was. However, unregistering things (like calling g_object_weak_unref()) should still reasonably work. Instead, now remove each weak notification one by one and invoke it. As we now invoke the callbacks in a loop, if a callee registers a new callback, then that one gets unregistered right away too. Previously, we would during g_object_real_dispose() only notify the notifications that were present when the loop starts. This is similar to what happens in closure_array_destroy_all(). This is a change in behavior, but it will be fixed in a separate follow-up commit. https://gitlab.gnome.org/GNOME/glib/-/issues/1002 --- gobject/gobject.c | 97 +++++++++++++++++++++++++++++--------- gobject/tests/references.c | 32 +++++++++++++ 2 files changed, 107 insertions(+), 22 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 4e6a65c51..07018494d 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -207,6 +207,7 @@ static guint object_floating_flag_handler (GObject *object, gint job); static inline void object_set_optional_flags (GObject *object, guint flags); +static void g_object_weak_release_all (GObject *object); static void object_interface_check_properties (gpointer check_data, gpointer g_iface); @@ -1827,7 +1828,7 @@ g_object_real_dispose (GObject *object) g_signal_handlers_destroy (object); /* GWeakNotify and GClosure can call into user code */ - g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL); + g_object_weak_release_all (object); closure_array_destroy_all (object); } @@ -3723,41 +3724,33 @@ typedef struct typedef struct { - GObject *object; guint n_weak_refs; WeakRefTuple weak_refs[1]; /* flexible array */ } WeakRefStack; -static void -weak_refs_notify (gpointer data) -{ - WeakRefStack *wstack = data; - guint i; - - for (i = 0; i < wstack->n_weak_refs; i++) - wstack->weak_refs[i].notify (wstack->weak_refs[i].data, wstack->object); - g_free (wstack); -} - static gpointer g_object_weak_ref_cb (gpointer *data, GDestroyNotify *destroy_notify, gpointer user_data) { - GObject *object = ((gpointer *) user_data)[0]; - GWeakNotify notify = ((gpointer *) user_data)[1]; - gpointer notify_data = ((gpointer *) user_data)[2]; + WeakRefTuple *tuple = user_data; WeakRefStack *wstack = *data; guint i; if (!wstack) { wstack = g_new (WeakRefStack, 1); - wstack->object = object; wstack->n_weak_refs = 1; i = 0; - *destroy_notify = weak_refs_notify; + /* We don't set a @destroy_notify. Shortly before finalize(), we call + * g_object_weak_release_all(), which frees the WeakRefStack. At that + * point the ref-count is already at zero and g_object_weak_ref() will + * assert against being called. This means, we expect that there is + * never anything to destroy. */ +#ifdef G_ENABLE_DEBUG + *destroy_notify = g_destroy_notify_assert_not_reached; +#endif } else { @@ -3767,8 +3760,7 @@ g_object_weak_ref_cb (gpointer *data, *data = wstack; - wstack->weak_refs[i].notify = notify; - wstack->weak_refs[i].data = notify_data; + wstack->weak_refs[i] = *tuple; return NULL; } @@ -3802,7 +3794,10 @@ g_object_weak_ref (GObject *object, _g_datalist_id_update_atomic (&object->qdata, quark_weak_notifies, g_object_weak_ref_cb, - ((gpointer[]){ object, notify, data })); + &((WeakRefTuple){ + .notify = notify, + .data = data, + })); } static gpointer @@ -3872,6 +3867,64 @@ g_object_weak_unref (GObject *object, })); } +static gpointer +g_object_weak_release_all_cb (gpointer *data, + GDestroyNotify *destroy_notify, + gpointer user_data) +{ + WeakRefTuple *tuple = user_data; + WeakRefStack *wstack = *data; + + if (!wstack) + return NULL; + +#ifdef G_ENABLE_DEBUG + g_assert (wstack->n_weak_refs > 0); +#endif + + wstack->n_weak_refs--; + + /* Emit the notifications in FIFO order. */ + *tuple = wstack->weak_refs[0]; + + if (wstack->n_weak_refs == 0) + { + g_free (wstack); + *data = NULL; + } + else + { + memmove (&wstack->weak_refs[0], + &wstack->weak_refs[1], + sizeof (wstack->weak_refs[0]) * wstack->n_weak_refs); + } + + return tuple; +} + +static void +g_object_weak_release_all (GObject *object) +{ + WeakRefTuple tuple; + + /* We notify weak references in a loop. As this emits external callbacks, a + * callee could register another weak reference, which the loop would notify + * right away. This means, registering new weak references during dispose + * does not work well, which you might want to do when resurrecting the + * object under destruction. + * + * This is an intentional choice. It would be complicated to keep track of + * the tuples that were present when the loop starts, and only notify those. + * + * You are advised to not register new weak references while handling a weak + * notification. */ + while (_g_datalist_id_update_atomic (&object->qdata, + quark_weak_notifies, + g_object_weak_release_all_cb, + &tuple)) + tuple.notify (tuple.data, object); +} + /** * g_object_add_weak_pointer: (skip) * @object: The object that should be weak referenced. @@ -4613,7 +4666,7 @@ retry_decrement: closure_array_destroy_all (object); g_signal_handlers_destroy (object); - g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL); + g_object_weak_release_all (object); TRACE (GOBJECT_OBJECT_FINALIZE (object, G_TYPE_FROM_INSTANCE (object))); G_OBJECT_GET_CLASS (object)->finalize (object); diff --git a/gobject/tests/references.c b/gobject/tests/references.c index ebd27f170..69e1984e9 100644 --- a/gobject/tests/references.c +++ b/gobject/tests/references.c @@ -337,6 +337,37 @@ test_references_many (void) /*****************************************************************************/ +static void notify_two (gpointer data, GObject *former_object); + +static void +notify_one (gpointer data, GObject *former_object) +{ + g_object_weak_unref (data, notify_two, former_object); +} + +static void +notify_two (gpointer data, GObject *former_object) +{ + g_assert_not_reached (); +} + +static void +test_references_two (void) +{ + GObject *obj; + + /* https://gitlab.gnome.org/GNOME/gtk/-/issues/5542#note_1688809 */ + + obj = g_object_new (G_TYPE_OBJECT, NULL); + + g_object_weak_ref (obj, notify_one, obj); + g_object_weak_ref (obj, notify_two, obj); + + g_object_unref (obj); +} + +/*****************************************************************************/ + int main (int argc, char *argv[]) @@ -349,6 +380,7 @@ main (int argc, g_test_add_func ("/gobject/references", test_references); g_test_add_func ("/gobject/references-many", test_references_many); + g_test_add_func ("/gobject/references_two", test_references_two); return g_test_run (); }