diff --git a/gobject/gobject.c b/gobject/gobject.c index 788bb0be2..c1d41d29b 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3786,23 +3786,29 @@ 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); - if (old_ref > 1) + retry_atomic_decrement1: + 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 ((int *)&object->ref_count, old_ref, old_ref - 1)) - goto retry_atomic_decrement1; + if (!g_atomic_int_compare_and_exchange_full ((int *)&object->ref_count, + old_ref, old_ref - 1, + &old_ref)) + 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); + 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; } - else + { GSList **weak_locations; GObjectNotifyQueue *nqueue; @@ -3865,24 +3871,29 @@ 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); - 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); - if (!g_atomic_int_compare_and_exchange ((int *)&object->ref_count, old_ref, old_ref - 1)) - goto retry_atomic_decrement2; + if (!g_atomic_int_compare_and_exchange_full ((int *)&object->ref_count, + old_ref, old_ref - 1, + &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 && 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) && + 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); + } return; } diff --git a/gobject/tests/reference.c b/gobject/tests/reference.c index 92080b7ff..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++; } @@ -779,6 +784,383 @@ 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; + gboolean disposing_refs_all_normal; + + 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 && !obj->disposing_refs_all_normal) + { + 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 void +toggle_notify_counter (gpointer data, + GObject *obj, + gboolean is_last) +{ + 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 +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 +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) +{ + 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, ==, 7); + g_assert_cmpuint (obj->notify_called, ==, 3); + + 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 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); +} + static gboolean global_destroyed; static gint global_value; @@ -982,6 +1364,8 @@ 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/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);