From c95bf0514cf2474bd88c2753601ce96e4a1bfa00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 29 Nov 2022 20:53:06 +0100 Subject: [PATCH 1/9] gobject: Use compare and exchange full to re-read old ref value In case g_atomic_int_compare_and_exchange() check fails we ended up doing another atomic get to figure out what it was the old reference count, however, we can avoid this by using the full version of the function that returns the value before the exchange happened as an out value. --- gobject/gobject.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index c1a69a896..95d3375c4 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3789,14 +3789,16 @@ g_object_unref (gpointer _object) g_return_if_fail (G_IS_OBJECT (object)); /* here we want to atomically do: if (ref_count>1) { ref_count--; return; } */ - retry_atomic_decrement1: old_ref = g_atomic_int_get (&object->ref_count); + retry_atomic_decrement1: if (old_ref > 1) { /* valid if last 2 refs are owned by this call to unref and the toggle_ref */ gboolean has_toggle_ref = OBJECT_HAS_TOGGLE_REF (object); - if (!g_atomic_int_compare_and_exchange ((int *)&object->ref_count, old_ref, old_ref - 1)) + if (!g_atomic_int_compare_and_exchange_full ((int *)&object->ref_count, + old_ref, old_ref - 1, + &old_ref)) goto retry_atomic_decrement1; TRACE (GOBJECT_OBJECT_UNREF(object,G_TYPE_FROM_INSTANCE(object),old_ref)); @@ -3868,14 +3870,16 @@ g_object_unref (gpointer _object) TRACE (GOBJECT_OBJECT_DISPOSE_END(object,G_TYPE_FROM_INSTANCE(object), 1)); /* may have been re-referenced meanwhile */ - retry_atomic_decrement2: old_ref = g_atomic_int_get ((int *)&object->ref_count); + retry_atomic_decrement2: if (old_ref > 1) { /* valid if last 2 refs are owned by this call to unref and the toggle_ref */ gboolean has_toggle_ref = OBJECT_HAS_TOGGLE_REF (object); - if (!g_atomic_int_compare_and_exchange ((int *)&object->ref_count, old_ref, old_ref - 1)) + if (!g_atomic_int_compare_and_exchange_full ((int *)&object->ref_count, + old_ref, old_ref - 1, + &old_ref)) goto retry_atomic_decrement2; /* emit all notifications that have been queued during dispose() */ From a89048c4f1f630e38a00946a60a181db6c96521e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 29 Nov 2022 21:35:04 +0100 Subject: [PATCH 2/9] gobject: Use a while instead of goto to repeat atomic increment We can use a cleaner solution now that we do not require to init the same value multiple times in the same way. --- gobject/gobject.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 95d3375c4..8308a55b3 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3871,8 +3871,8 @@ g_object_unref (gpointer _object) /* may have been re-referenced meanwhile */ old_ref = g_atomic_int_get ((int *)&object->ref_count); - retry_atomic_decrement2: - if (old_ref > 1) + + while (old_ref > 1) { /* valid if last 2 refs are owned by this call to unref and the toggle_ref */ gboolean has_toggle_ref = OBJECT_HAS_TOGGLE_REF (object); @@ -3880,7 +3880,7 @@ g_object_unref (gpointer _object) if (!g_atomic_int_compare_and_exchange_full ((int *)&object->ref_count, old_ref, old_ref - 1, &old_ref)) - goto retry_atomic_decrement2; + continue; /* emit all notifications that have been queued during dispose() */ g_object_notify_queue_thaw (object, nqueue); From 65303537b0ed0add418da206024f6d87429c788a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 29 Nov 2022 21:50:30 +0100 Subject: [PATCH 3/9] gobject: Remove initial goto to repeat unref operation --- gobject/gobject.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 8308a55b3..43d337d1b 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3791,7 +3791,7 @@ g_object_unref (gpointer _object) /* here we want to atomically do: if (ref_count>1) { ref_count--; return; } */ old_ref = g_atomic_int_get (&object->ref_count); retry_atomic_decrement1: - if (old_ref > 1) + while (old_ref > 1) { /* valid if last 2 refs are owned by this call to unref and the toggle_ref */ gboolean has_toggle_ref = OBJECT_HAS_TOGGLE_REF (object); @@ -3799,15 +3799,17 @@ g_object_unref (gpointer _object) if (!g_atomic_int_compare_and_exchange_full ((int *)&object->ref_count, old_ref, old_ref - 1, &old_ref)) - goto retry_atomic_decrement1; + continue; TRACE (GOBJECT_OBJECT_UNREF(object,G_TYPE_FROM_INSTANCE(object),old_ref)); /* if we went from 2->1 we need to notify toggle refs if any */ if (old_ref == 2 && has_toggle_ref) /* The last ref being held in this case is owned by the toggle_ref */ toggle_refs_notify (object, TRUE); + + return; } - else + { GSList **weak_locations; GObjectNotifyQueue *nqueue; From c0360f626cecd0b5612373d8e4989405e3168db8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 29 Nov 2022 21:36:48 +0100 Subject: [PATCH 4/9] gobject: Read the toggle reference state only after we've update the references We were reading if an object has toggle references even if this was not really relevant for the current object state, as we only need to notify when going from 2 to 1 references, so first ensure that this is the case and then check if we have toggle references enabled in the object. This is a micro-optimization, for the way flags are defined, but still an operation we can avoid in most cases. --- gobject/gobject.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 43d337d1b..6f0149d12 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3794,7 +3794,6 @@ g_object_unref (gpointer _object) while (old_ref > 1) { /* valid if last 2 refs are owned by this call to unref and the toggle_ref */ - gboolean has_toggle_ref = OBJECT_HAS_TOGGLE_REF (object); if (!g_atomic_int_compare_and_exchange_full ((int *)&object->ref_count, old_ref, old_ref - 1, @@ -3804,8 +3803,11 @@ g_object_unref (gpointer _object) TRACE (GOBJECT_OBJECT_UNREF(object,G_TYPE_FROM_INSTANCE(object),old_ref)); /* if we went from 2->1 we need to notify toggle refs if any */ - if (old_ref == 2 && has_toggle_ref) /* The last ref being held in this case is owned by the toggle_ref */ - toggle_refs_notify (object, TRUE); + if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object)) + { + /* The last ref being held in this case is owned by the toggle_ref */ + toggle_refs_notify (object, TRUE); + } return; } From ea0c4d45b2a9e54a9258fd03cfb82494520a628d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 29 Nov 2022 23:12:17 +0100 Subject: [PATCH 5/9] gobject/tests/reference: Add test for toggle reference up/down during dispose --- gobject/tests/reference.c | 235 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 235 insertions(+) diff --git a/gobject/tests/reference.c b/gobject/tests/reference.c index 92080b7ff..20bd7a5bb 100644 --- a/gobject/tests/reference.c +++ b/gobject/tests/reference.c @@ -779,6 +779,240 @@ test_toggle_ref (void) g_object_remove_toggle_ref (obj, toggle_notify, &c); } +G_DECLARE_FINAL_TYPE (DisposeReffingObject, dispose_reffing_object, + DISPOSE, REFFING_OBJECT, GObject) + +typedef enum +{ + PROP_INT_PROP = 1, + N_PROPS, +} DisposeReffingObjectProperty; + +static GParamSpec *dispose_reffing_object_properties[N_PROPS] = {0}; + +struct _DisposeReffingObject +{ + GObject parent; + + GToggleNotify toggle_notify; + Count actual; + Count expected; + unsigned disposing_refs; + + GCallback notify_handler; + unsigned notify_called; + + int int_prop; + + GWeakRef *weak_ref; +}; + +G_DEFINE_TYPE (DisposeReffingObject, dispose_reffing_object, G_TYPE_OBJECT) + +static void +on_object_notify (GObject *object, + GParamSpec *pspec, + void *data) +{ + DisposeReffingObject *obj = DISPOSE_REFFING_OBJECT (object); + + obj->notify_called++; +} + +static void +dispose_reffing_object_dispose (GObject *object) +{ + DisposeReffingObject *obj = DISPOSE_REFFING_OBJECT (object); + + g_assert_cmpint (object->ref_count, ==, 1); + g_assert_cmpint (obj->actual.count, ==, obj->expected.count); + + for (unsigned i = 0; i < obj->disposing_refs; ++i) + { + if (i == 0) + { + g_object_add_toggle_ref (object, obj->toggle_notify, &obj->actual); + } + else + { + obj->actual.should_be_last = FALSE; + g_object_ref (obj); + g_assert_cmpint (obj->actual.count, ==, obj->expected.count); + } + + obj->actual.should_be_last = TRUE; + } + + G_OBJECT_CLASS (dispose_reffing_object_parent_class)->dispose (object); + + if (obj->notify_handler) + { + unsigned old_notify_called = obj->notify_called; + + g_assert_cmpuint (g_signal_handler_find (object, G_SIGNAL_MATCH_FUNC, + 0, 0, NULL, obj->notify_handler, NULL), ==, 0); + + g_signal_connect (object, "notify", G_CALLBACK (obj->notify_handler), NULL); + + /* This would trigger a toggle notification, but is not something we may + * want with https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2377 + * so, we only test this in case we have more than one ref + */ + if (obj->toggle_notify == toggle_notify) + g_assert_cmpint (obj->disposing_refs, >, 1); + + g_object_notify (object, "int-prop"); + g_assert_cmpuint (obj->notify_called, ==, old_notify_called); + } + + g_assert_cmpint (obj->actual.count, ==, obj->expected.count); +} + +static void +dispose_reffing_object_init (DisposeReffingObject *connector) +{ +} + +static void +dispose_reffing_object_set_property (GObject *object, + guint property_id, + const GValue *value, + GParamSpec *pspec) +{ + DisposeReffingObject *obj = DISPOSE_REFFING_OBJECT (object); + + switch ((DisposeReffingObjectProperty) property_id) + { + case PROP_INT_PROP: + obj->int_prop = g_value_get_int (value); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); + break; + } +} + +static void +dispose_reffing_object_get_property (GObject *object, + guint property_id, + GValue *value, + GParamSpec *pspec) +{ + DisposeReffingObject *obj = DISPOSE_REFFING_OBJECT (object); + + switch ((DisposeReffingObjectProperty) property_id) + { + case PROP_INT_PROP: + g_value_set_int (value, obj->int_prop); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); + break; + } +} + +static void +dispose_reffing_object_class_init (DisposeReffingObjectClass *klass) +{ + GObjectClass *object_class = G_OBJECT_CLASS (klass); + + dispose_reffing_object_properties[PROP_INT_PROP] = + g_param_spec_int ("int-prop", "int-prop", "int-prop", + G_MININT, G_MAXINT, + 0, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS); + + object_class->dispose = dispose_reffing_object_dispose; + object_class->set_property = dispose_reffing_object_set_property; + object_class->get_property = dispose_reffing_object_get_property; + + g_object_class_install_properties (object_class, N_PROPS, + dispose_reffing_object_properties); +} + +static void +test_toggle_ref_on_dispose (void) +{ + DisposeReffingObject *obj; + gpointer disposed_checker = &obj; + + /* This tests wants to ensure that an object that gets re-referenced + * (one or multiple times) during its dispose virtual function: + * - Notifies all the queued "notify" signal handlers + * - Notifies toggle notifications if any + * - It does not get finalized + */ + + obj = g_object_new (dispose_reffing_object_get_type (), NULL); + obj->toggle_notify = toggle_notify; + obj->notify_handler = G_CALLBACK (on_object_notify); + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + /* Convert to toggle notification */ + g_object_add_toggle_ref (G_OBJECT (obj), obj->toggle_notify, &obj->actual); + g_assert_cmpint (obj->actual.count, ==, 0); + + obj->actual.should_be_last = TRUE; + obj->notify_handler = G_CALLBACK (on_object_notify); + g_object_unref (obj); + g_assert_cmpint (obj->actual.count, ==, 1); + g_assert_cmpuint (obj->notify_called, ==, 0); + + /* Remove the toggle reference, making it to dispose and resurrect again */ + obj->disposing_refs = 1; + obj->expected.count = 1; + obj->notify_handler = NULL; /* FIXME: enable it when !2377 is in */ + g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL); + g_assert_cmpint (obj->actual.count, ==, 2); + g_assert_cmpuint (obj->notify_called, ==, 0); + + g_assert_null (disposed_checker); + g_assert_cmpint (g_atomic_int_get (&G_OBJECT (obj)->ref_count), ==, + obj->disposing_refs); + + /* Object has been disposed, but is still alive, so add another weak pointer */ + disposed_checker = &obj; + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + /* Remove the toggle reference, making it to dispose and resurrect with + * more references than before, so that no toggle notify is called + */ + obj->disposing_refs = 3; + obj->expected.count = 2; + obj->notify_handler = G_CALLBACK (on_object_notify); + g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL); + g_assert_cmpint (obj->actual.count, ==, 2); + g_assert_cmpint (obj->notify_called, ==, 1); + obj->expected.count = obj->actual.count; + + g_assert_null (disposed_checker); + g_assert_cmpint (g_atomic_int_get (&G_OBJECT (obj)->ref_count), ==, + obj->disposing_refs); + + disposed_checker = &obj; + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + /* Now remove the first added reference */ + obj->disposing_refs = 0; + g_object_unref (obj); + g_assert_nonnull (disposed_checker); + g_assert_cmpint (g_atomic_int_get (&G_OBJECT (obj)->ref_count), ==, 2); + g_assert_cmpint (obj->actual.count, ==, 2); + g_assert_cmpint (obj->notify_called, ==, 1); + + /* And the toggle one */ + obj->actual.should_be_last = TRUE; + obj->notify_handler = NULL; + g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL); + g_assert_nonnull (disposed_checker); + g_assert_cmpint (g_atomic_int_get (&G_OBJECT (obj)->ref_count), ==, 1); + g_assert_cmpint (obj->actual.count, ==, 2); + obj->expected.count = obj->actual.count; + + g_clear_object (&obj); + g_assert_null (disposed_checker); +} + static gboolean global_destroyed; static gint global_value; @@ -982,6 +1216,7 @@ main (int argc, char **argv) g_test_add_func ("/object/weak-ref/on-run-dispose", test_weak_ref_on_run_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/toggle-ref/ref-on-dispose", test_toggle_ref_on_dispose); g_test_add_func ("/object/qdata", test_object_qdata); g_test_add_func ("/object/qdata2", test_object_qdata2); From 5e2b288033922e969ab5e44ca5310ccbbd1127f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 6 Dec 2022 03:51:09 +0100 Subject: [PATCH 6/9] gobject/tests/reference: Add test for notify during dispose We need to check whether notifications and toggle references are working properly if an object gets revitalized during the dispose vfunc. --- gobject/tests/reference.c | 112 +++++++++++++++++++++++++++++++++++++- 1 file changed, 111 insertions(+), 1 deletion(-) diff --git a/gobject/tests/reference.c b/gobject/tests/reference.c index 20bd7a5bb..ed15e6b34 100644 --- a/gobject/tests/reference.c +++ b/gobject/tests/reference.c @@ -798,6 +798,7 @@ struct _DisposeReffingObject Count actual; Count expected; unsigned disposing_refs; + gboolean disposing_refs_all_normal; GCallback notify_handler; unsigned notify_called; @@ -829,7 +830,7 @@ dispose_reffing_object_dispose (GObject *object) for (unsigned i = 0; i < obj->disposing_refs; ++i) { - if (i == 0) + if (i == 0 && !obj->disposing_refs_all_normal) { g_object_add_toggle_ref (object, obj->toggle_notify, &obj->actual); } @@ -1013,6 +1014,114 @@ test_toggle_ref_on_dispose (void) g_assert_null (disposed_checker); } +static void +toggle_notify_counter (gpointer data, + GObject *obj, + gboolean is_last) +{ + Count *c = data; + c->count++; +} + +static void +on_object_notify_switch_to_normal_ref (GObject *object, + GParamSpec *pspec, + void *data) +{ + DisposeReffingObject *obj = DISPOSE_REFFING_OBJECT (object); + + obj->notify_called++; + + g_object_ref (object); + g_object_remove_toggle_ref (object, obj->toggle_notify, NULL); +} + +static void +on_object_notify_switch_to_toggle_ref (GObject *object, + GParamSpec *pspec, + void *data) +{ + DisposeReffingObject *obj = DISPOSE_REFFING_OBJECT (object); + + obj->notify_called++; + + g_object_add_toggle_ref (object, obj->toggle_notify, &obj->actual); + g_object_unref (object); +} + +static void +test_toggle_ref_and_notify_on_dispose (void) +{ + DisposeReffingObject *obj; + gpointer disposed_checker = &obj; + + /* This tests wants to ensure that toggle signal emission during dispose + * is properly working if the object is revitalized by adding new references. + * It also wants to check that toggle notifications are not happening if a + * notify handler is removing them at this phase. + */ + + obj = g_object_new (dispose_reffing_object_get_type (), NULL); + obj->toggle_notify = toggle_notify_counter; + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + /* Convert to toggle notification */ + g_object_add_toggle_ref (G_OBJECT (obj), obj->toggle_notify, &obj->actual); + g_assert_cmpint (obj->actual.count, ==, 0); + + obj->notify_handler = G_CALLBACK (on_object_notify); + g_object_unref (obj); + g_assert_cmpint (obj->actual.count, ==, 1); + g_assert_cmpuint (obj->notify_called, ==, 0); + + disposed_checker = &obj; + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + /* Check that notification is triggered after being queued */ + obj->disposing_refs = 1; + obj->expected.count = 1; + obj->notify_handler = G_CALLBACK (on_object_notify); + g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL); + /* FIXME: adjust the count to 1 when !2377 is in */ + g_assert_cmpint (obj->actual.count, ==, 4); + g_assert_cmpuint (obj->notify_called, ==, 1); + + disposed_checker = &obj; + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + /* Check that notification is triggered after being queued, but no toggle + * notification is happening if notify handler switches to normal reference + */ + obj->disposing_refs = 1; + obj->expected.count = 4; + obj->notify_handler = G_CALLBACK (on_object_notify_switch_to_normal_ref); + g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL); + g_assert_cmpint (obj->actual.count, ==, 5); + g_assert_cmpuint (obj->notify_called, ==, 2); + + disposed_checker = &obj; + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + /* Check that notification is triggered after being queued, but that toggle + * is happening if notify handler switched to toggle reference + */ + obj->disposing_refs = 1; + obj->disposing_refs_all_normal = TRUE; + obj->expected.count = 5; + obj->notify_handler = G_CALLBACK (on_object_notify_switch_to_toggle_ref); + g_object_unref (obj); + g_assert_cmpint (obj->actual.count, ==, 6); + g_assert_cmpuint (obj->notify_called, ==, 3); + + disposed_checker = &obj; + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + obj->disposing_refs = 0; + obj->expected.count = 6; + g_clear_object (&obj); + g_assert_null (disposed_checker); +} + static gboolean global_destroyed; static gint global_value; @@ -1217,6 +1326,7 @@ main (int argc, char **argv) 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/toggle-ref/ref-on-dispose", test_toggle_ref_on_dispose); + g_test_add_func ("/object/toggle-ref/ref-and-notify-on-dispose", test_toggle_ref_and_notify_on_dispose); g_test_add_func ("/object/qdata", test_object_qdata); g_test_add_func ("/object/qdata2", test_object_qdata2); From 1f852863ec65cca8962fc0e94df60728aa296fd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 6 Dec 2022 04:07:54 +0100 Subject: [PATCH 7/9] gobject: Check for toggle references only if the old ref is relevant If an object gets revitalized during the dispose vfunc, we need to call toggle refs notifiers only if we had 2 references and if the object has the toggle references enabled. This may change in case an object notifier handler changes this status, so do this check only after we've called the notifiers so that in case toggle notifications are enabled afterwards we still call the handlers. --- gobject/gobject.c | 8 +++++--- gobject/tests/reference.c | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 6f0149d12..fca5fa19f 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3879,7 +3879,6 @@ g_object_unref (gpointer _object) while (old_ref > 1) { /* valid if last 2 refs are owned by this call to unref and the toggle_ref */ - gboolean has_toggle_ref = OBJECT_HAS_TOGGLE_REF (object); if (!g_atomic_int_compare_and_exchange_full ((int *)&object->ref_count, old_ref, old_ref - 1, @@ -3892,8 +3891,11 @@ g_object_unref (gpointer _object) TRACE (GOBJECT_OBJECT_UNREF(object,G_TYPE_FROM_INSTANCE(object),old_ref)); /* if we went from 2->1 we need to notify toggle refs if any */ - if (old_ref == 2 && has_toggle_ref) /* The last ref being held in this case is owned by the toggle_ref */ - toggle_refs_notify (object, TRUE); + if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object)) + { + /* The last ref being held in this case is owned by the toggle_ref */ + toggle_refs_notify (object, TRUE); + } return; } diff --git a/gobject/tests/reference.c b/gobject/tests/reference.c index ed15e6b34..46760fd0b 100644 --- a/gobject/tests/reference.c +++ b/gobject/tests/reference.c @@ -1110,14 +1110,14 @@ test_toggle_ref_and_notify_on_dispose (void) obj->expected.count = 5; obj->notify_handler = G_CALLBACK (on_object_notify_switch_to_toggle_ref); g_object_unref (obj); - g_assert_cmpint (obj->actual.count, ==, 6); + g_assert_cmpint (obj->actual.count, ==, 7); g_assert_cmpuint (obj->notify_called, ==, 3); disposed_checker = &obj; g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); obj->disposing_refs = 0; - obj->expected.count = 6; + obj->expected.count = 7; g_clear_object (&obj); g_assert_null (disposed_checker); } From 0918ce013af0b7d5ac1621fbbbb3dd114f1b7c64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 6 Dec 2022 04:27:53 +0100 Subject: [PATCH 8/9] gobject: Do not call toggle down notifications if current refcount is not 1 When an object is revitalized and a notify callbacks increased the reference counter of the object, we are calling the toggle notifier twice, while it should only happen if also the actual reference count value is 1 (after having been decremented from 2). --- gobject/gobject.c | 3 ++- gobject/tests/reference.c | 41 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index fca5fa19f..da799888d 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3891,7 +3891,8 @@ g_object_unref (gpointer _object) TRACE (GOBJECT_OBJECT_UNREF(object,G_TYPE_FROM_INSTANCE(object),old_ref)); /* if we went from 2->1 we need to notify toggle refs if any */ - if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object)) + if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object) && + g_atomic_int_get ((int *)&object->ref_count) == 1) { /* The last ref being held in this case is owned by the toggle_ref */ toggle_refs_notify (object, TRUE); diff --git a/gobject/tests/reference.c b/gobject/tests/reference.c index 46760fd0b..fa85ef997 100644 --- a/gobject/tests/reference.c +++ b/gobject/tests/reference.c @@ -731,6 +731,11 @@ toggle_notify (gpointer data, g_assert (is_last == c->should_be_last); + if (is_last) + g_assert_cmpint (g_atomic_int_get (&obj->ref_count), ==, 1); + else + g_assert_cmpint (g_atomic_int_get (&obj->ref_count), ==, 2); + c->count++; } @@ -1021,6 +1026,11 @@ toggle_notify_counter (gpointer data, { Count *c = data; c->count++; + + if (is_last) + g_assert_cmpint (g_atomic_int_get (&obj->ref_count), ==, 1); + else + g_assert_cmpint (g_atomic_int_get (&obj->ref_count), ==, 2); } static void @@ -1049,6 +1059,20 @@ on_object_notify_switch_to_toggle_ref (GObject *object, g_object_unref (object); } +static void +on_object_notify_add_ref (GObject *object, + GParamSpec *pspec, + void *data) +{ + DisposeReffingObject *obj = DISPOSE_REFFING_OBJECT (object); + int old_toggle_cout = obj->actual.count; + + obj->notify_called++; + + g_object_ref (object); + g_assert_cmpint (obj->actual.count, ==, old_toggle_cout); +} + static void test_toggle_ref_and_notify_on_dispose (void) { @@ -1116,8 +1140,23 @@ test_toggle_ref_and_notify_on_dispose (void) disposed_checker = &obj; g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); - obj->disposing_refs = 0; + /* Check that notification is triggered after being queued, but that toggle + * is not happening if current refcount changed. + */ + obj->disposing_refs = 1; + obj->disposing_refs_all_normal = FALSE; obj->expected.count = 7; + obj->notify_handler = G_CALLBACK (on_object_notify_add_ref); + g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL); + g_assert_cmpint (obj->actual.count, ==, 8); + g_assert_cmpuint (obj->notify_called, ==, 4); + g_object_unref (obj); + + disposed_checker = &obj; + g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker); + + obj->disposing_refs = 0; + obj->expected.count = 9; g_clear_object (&obj); g_assert_null (disposed_checker); } From 6cd6cc41bbb3c35be33972e501cdb56282faec25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 6 Dec 2022 04:43:02 +0100 Subject: [PATCH 9/9] gobject: Trace unref just after this happened Trace the unref before potentially calling (user) code that may re-ref or unref again, causing the tracing order to be messed up. --- gobject/gobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index da799888d..ee2cea865 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3885,11 +3885,11 @@ g_object_unref (gpointer _object) &old_ref)) continue; + TRACE (GOBJECT_OBJECT_UNREF (object, G_TYPE_FROM_INSTANCE (object), old_ref)); + /* emit all notifications that have been queued during dispose() */ g_object_notify_queue_thaw (object, nqueue); - TRACE (GOBJECT_OBJECT_UNREF(object,G_TYPE_FROM_INSTANCE(object),old_ref)); - /* if we went from 2->1 we need to notify toggle refs if any */ if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object) && g_atomic_int_get ((int *)&object->ref_count) == 1)