From 8c4598aab31759e3c296b54f39630d028ce2334a Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Wed, 22 Jun 2022 12:51:07 -0400 Subject: [PATCH 01/11] dataset: Do not increment Do not increment the `data` pointer when it points to an item that has not been inspected. Helps https://gitlab.gnome.org/GNOME/glib/-/issues/2672 --- glib/gdataset.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/glib/gdataset.c b/glib/gdataset.c index 6b78d2e33..cc19c9674 100644 --- a/glib/gdataset.c +++ b/glib/gdataset.c @@ -528,11 +528,8 @@ g_data_remove_internal (GData **datalist, old[found_keys] = *data; found_keys++; - if (data < data_end) - { - data_end--; - *data = *data_end; - } + if (data < --data_end) + *data = *data_end; d->len--; @@ -546,8 +543,10 @@ g_data_remove_internal (GData **datalist, break; } } - - data++; + else + { + data++; + } } if (found_keys > 0) From ad0f7199f19fb55b2196721bf90248d5587825c7 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Wed, 22 Jun 2022 15:09:41 -0400 Subject: [PATCH 02/11] glib/tests/dataset: Add a test Test that `g_datalist_id_remove_multiple()` removes all the keys it is given. Helps https://gitlab.gnome.org/GNOME/glib/-/issues/2672 --- glib/tests/dataset.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/glib/tests/dataset.c b/glib/tests/dataset.c index b02b3e42b..1d6feea82 100644 --- a/glib/tests/dataset.c +++ b/glib/tests/dataset.c @@ -250,6 +250,46 @@ test_datalist_id (void) g_datalist_clear (&list); } +static void +foreach_func (GQuark key_id, + gpointer data, + gpointer user_data) +{ + int *count = user_data; + + (*count)++; +} + +static void +test_datalist_id_remove_multiple (void) +{ + /* Test that g_datalist_id_remove_multiple() removes all the keys it + * is given. */ + GData *list = NULL; + GQuark one = g_quark_from_static_string ("one"); + GQuark two = g_quark_from_static_string ("two"); + GQuark three = g_quark_from_static_string ("three"); + GQuark keys[] = { + one, + two, + three, + }; + int count; + + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/issues/2672"); + + g_datalist_init (&list); + g_datalist_id_set_data (&list, one, GINT_TO_POINTER (1)); + g_datalist_id_set_data (&list, two, GINT_TO_POINTER (2)); + g_datalist_id_set_data (&list, three, GINT_TO_POINTER (3)); + + g_datalist_id_remove_multiple (&list, keys, G_N_ELEMENTS (keys)); + + count = 0; + g_datalist_foreach (&list, foreach_func, &count); + g_assert_cmpint (count, ==, 0); +} + int main (int argc, char *argv[]) { @@ -265,6 +305,7 @@ main (int argc, char *argv[]) g_test_add_func ("/datalist/basic", test_datalist_basic); g_test_add_func ("/datalist/id", test_datalist_id); g_test_add_func ("/datalist/recursive-clear", test_datalist_clear); + g_test_add_func ("/datalist/id-remove-multiple", test_datalist_id_remove_multiple); return g_test_run (); } From ac113c1ed23bb02b4dbd1256cb01100336191ccf Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Thu, 23 Jun 2022 21:30:51 +0000 Subject: [PATCH 03/11] In `test_datalist_id_remove_multiple()`, verify that the data list contains 3 items before calling `g_datalist_id_remove_multiple()`. --- glib/tests/dataset.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/glib/tests/dataset.c b/glib/tests/dataset.c index 1d6feea82..12eb33ab3 100644 --- a/glib/tests/dataset.c +++ b/glib/tests/dataset.c @@ -283,6 +283,10 @@ test_datalist_id_remove_multiple (void) g_datalist_id_set_data (&list, two, GINT_TO_POINTER (2)); g_datalist_id_set_data (&list, three, GINT_TO_POINTER (3)); + count = 0; + g_datalist_foreach (&list, foreach_func, &count); + g_assert_cmpint (count, ==, 3); + g_datalist_id_remove_multiple (&list, keys, G_N_ELEMENTS (keys)); count = 0; From a4fa456e677246629e714d05b5de178691571b06 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Tue, 21 Jun 2022 15:44:37 -0400 Subject: [PATCH 04/11] gdataset: Preserve destruction order In `g_data_remove_internal()`, call the `GDataElt:destroy` functions in the order that they appear in `keys`, instead of the order that they are found in `datalist`. Fixes https://gitlab.gnome.org/GNOME/glib/-/issues/2672 --- glib/gdataset.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/glib/gdataset.c b/glib/gdataset.c index cc19c9674..2ac78b9d5 100644 --- a/glib/gdataset.c +++ b/glib/gdataset.c @@ -504,7 +504,7 @@ g_data_remove_internal (GData **datalist, GDataElt *old, *data, *data_end; gsize found_keys; - old = g_newa (GDataElt, n_keys); + old = g_newa0 (GDataElt, n_keys); data = d->data; data_end = data + d->len; @@ -518,6 +518,7 @@ g_data_remove_internal (GData **datalist, { if (data->key == keys[i]) { + old[i] = *data; remove = TRUE; break; } @@ -525,7 +526,6 @@ g_data_remove_internal (GData **datalist, if (remove) { - old[found_keys] = *data; found_keys++; if (data < --data_end) @@ -553,7 +553,7 @@ g_data_remove_internal (GData **datalist, { g_datalist_unlock (datalist); - for (gsize i = 0; i < found_keys; i++) + for (gsize i = 0; i < n_keys; i++) { if (old[i].destroy) old[i].destroy (old[i].data); From 42826576a882930c48bd7d95bc07f3354e51fde1 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Tue, 21 Jun 2022 18:43:30 -0400 Subject: [PATCH 05/11] glib/tests/dataset: Test id_remove_multiple Test that destroy-funcs are called in the order that the keys are specified, not the order that they are found in the datalist. Helps https://gitlab.gnome.org/GNOME/glib/-/issues/2672 --- glib/tests/dataset.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/glib/tests/dataset.c b/glib/tests/dataset.c index 12eb33ab3..2897e6159 100644 --- a/glib/tests/dataset.c +++ b/glib/tests/dataset.c @@ -294,6 +294,41 @@ test_datalist_id_remove_multiple (void) g_assert_cmpint (count, ==, 0); } +static void +destroy_func (gpointer data) +{ + static int i = 0; + + i++; + g_assert_cmpint (GPOINTER_TO_INT (data), ==, i); +} + +static void +test_datalist_id_remove_multiple_destroy_order (void) +{ + /* Test that destroy-funcs are called in the order that the keys are + * specified, not the order that they are found in the datalist. */ + GData *list = NULL; + GQuark one = g_quark_from_static_string ("one"); + GQuark two = g_quark_from_static_string ("two"); + GQuark three = g_quark_from_static_string ("three"); + GQuark keys[] = { + one, + two, + three, + }; + + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/issues/2672"); + + g_datalist_init (&list); + + g_datalist_id_set_data_full (&list, two, GINT_TO_POINTER (2), destroy_func); + g_datalist_id_set_data_full (&list, three, GINT_TO_POINTER (3), destroy_func); + g_datalist_id_set_data_full (&list, one, GINT_TO_POINTER (1), destroy_func); + + g_datalist_id_remove_multiple (&list, keys, G_N_ELEMENTS (keys)); +} + int main (int argc, char *argv[]) { @@ -310,6 +345,8 @@ main (int argc, char *argv[]) g_test_add_func ("/datalist/id", test_datalist_id); g_test_add_func ("/datalist/recursive-clear", test_datalist_clear); g_test_add_func ("/datalist/id-remove-multiple", test_datalist_id_remove_multiple); + g_test_add_func ("/datalist/id-remove-multiple-destroy-order", + test_datalist_id_remove_multiple_destroy_order); return g_test_run (); } From a437a97ffd8b398d5407baee2574ce9f9c013d70 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Thu, 23 Jun 2022 22:17:13 +0000 Subject: [PATCH 06/11] dataset: Rename `i` as `destroy_index` and move it out of `destroy_func()`, so that it can be checked to confirm that `destroy_func()` was called three times. --- glib/tests/dataset.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/glib/tests/dataset.c b/glib/tests/dataset.c index 2897e6159..cfc7a69c9 100644 --- a/glib/tests/dataset.c +++ b/glib/tests/dataset.c @@ -294,13 +294,13 @@ test_datalist_id_remove_multiple (void) g_assert_cmpint (count, ==, 0); } +static int destroy_index; + static void destroy_func (gpointer data) { - static int i = 0; - - i++; - g_assert_cmpint (GPOINTER_TO_INT (data), ==, i); + destroy_index++; + g_assert_cmpint (GPOINTER_TO_INT (data), ==, destroy_index); } static void @@ -326,7 +326,10 @@ test_datalist_id_remove_multiple_destroy_order (void) g_datalist_id_set_data_full (&list, three, GINT_TO_POINTER (3), destroy_func); g_datalist_id_set_data_full (&list, one, GINT_TO_POINTER (1), destroy_func); + destroy_index = 0; g_datalist_id_remove_multiple (&list, keys, G_N_ELEMENTS (keys)); + /* This verifies that destroy_func() was called three times: */ + g_assert_cmpint (destroy_index, ==, 3); } int From e95a6bdd7dbcf0d4732b4b1ccdb33ebd840e7b59 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Thu, 23 Jun 2022 20:52:59 -0400 Subject: [PATCH 07/11] glib/tests/dataset: Use existing code `destroy_index` duplicates `destroy_count`, and `foreach_func()` essentially duplicates `notify()`. --- glib/tests/dataset.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/glib/tests/dataset.c b/glib/tests/dataset.c index cfc7a69c9..7f7db13dc 100644 --- a/glib/tests/dataset.c +++ b/glib/tests/dataset.c @@ -250,16 +250,6 @@ test_datalist_id (void) g_datalist_clear (&list); } -static void -foreach_func (GQuark key_id, - gpointer data, - gpointer user_data) -{ - int *count = user_data; - - (*count)++; -} - static void test_datalist_id_remove_multiple (void) { @@ -274,7 +264,6 @@ test_datalist_id_remove_multiple (void) two, three, }; - int count; g_test_bug ("https://gitlab.gnome.org/GNOME/glib/issues/2672"); @@ -283,24 +272,22 @@ test_datalist_id_remove_multiple (void) g_datalist_id_set_data (&list, two, GINT_TO_POINTER (2)); g_datalist_id_set_data (&list, three, GINT_TO_POINTER (3)); - count = 0; - g_datalist_foreach (&list, foreach_func, &count); - g_assert_cmpint (count, ==, 3); + destroy_count = 0; + g_datalist_foreach (&list, (GDataForeachFunc) notify, NULL); + g_assert_cmpint (destroy_count, ==, 3); g_datalist_id_remove_multiple (&list, keys, G_N_ELEMENTS (keys)); - count = 0; - g_datalist_foreach (&list, foreach_func, &count); - g_assert_cmpint (count, ==, 0); + destroy_count = 0; + g_datalist_foreach (&list, (GDataForeachFunc) notify, NULL); + g_assert_cmpint (destroy_count, ==, 0); } -static int destroy_index; - static void destroy_func (gpointer data) { - destroy_index++; - g_assert_cmpint (GPOINTER_TO_INT (data), ==, destroy_index); + destroy_count++; + g_assert_cmpint (GPOINTER_TO_INT (data), ==, destroy_count); } static void @@ -326,10 +313,10 @@ test_datalist_id_remove_multiple_destroy_order (void) g_datalist_id_set_data_full (&list, three, GINT_TO_POINTER (3), destroy_func); g_datalist_id_set_data_full (&list, one, GINT_TO_POINTER (1), destroy_func); - destroy_index = 0; + destroy_count = 0; g_datalist_id_remove_multiple (&list, keys, G_N_ELEMENTS (keys)); /* This verifies that destroy_func() was called three times: */ - g_assert_cmpint (destroy_index, ==, 3); + g_assert_cmpint (destroy_count, ==, 3); } int From 8bd63258a538ea59782cdb2e14614f1cfb3b3a37 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Mon, 27 Jun 2022 17:42:11 -0400 Subject: [PATCH 08/11] dataset: Improve readability Improve the readability by using a temporary variable --- glib/gdataset.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/glib/gdataset.c b/glib/gdataset.c index 2ac78b9d5..88306c389 100644 --- a/glib/gdataset.c +++ b/glib/gdataset.c @@ -526,11 +526,14 @@ g_data_remove_internal (GData **datalist, if (remove) { + GDataElt *data_last = data_end - 1; + found_keys++; - if (data < --data_end) - *data = *data_end; + if (data < data_last) + *data = *data_last; + data_end--; d->len--; /* We don't bother to shrink, but if all data are now gone From 8a43ae71c898bce3f8f9ed9dc9d7065489614291 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Mon, 27 Jun 2022 17:53:25 -0400 Subject: [PATCH 09/11] dataset: Document some logic Add comments to clarify how the 'old' array is allocated and used. --- glib/gdataset.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/glib/gdataset.c b/glib/gdataset.c index 88306c389..500022630 100644 --- a/glib/gdataset.c +++ b/glib/gdataset.c @@ -504,6 +504,11 @@ g_data_remove_internal (GData **datalist, GDataElt *old, *data, *data_end; gsize found_keys; + /* Allocate an array of GDataElt to hold copies of the elements + * that are removed from the datalist. Allow enough space for all + * the keys; if a key is not found, the corresponding element of + * old is not populated, so we initialize them all to NULL to + * detect that case. */ old = g_newa0 (GDataElt, n_keys); data = d->data; @@ -558,6 +563,9 @@ g_data_remove_internal (GData **datalist, for (gsize i = 0; i < n_keys; i++) { + /* If keys[i] was not found, then old[i].destroy is NULL. + * Call old[i].destroy() only if keys[i] was found, and + * is associated with a destroy notifier: */ if (old[i].destroy) old[i].destroy (old[i].data); } From 94ba14d5424c9304342a601cb9eb0842e56d8f51 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Sun, 3 Jul 2022 14:56:44 -0400 Subject: [PATCH 10/11] gobject: Weaken an assertion in g_weak_ref_set() When weak references are being cleaned up, it is possible for the `qdata` for both `quark_weak_locations` and `quark_weak_refs` to have been deallocated, so that `g_datalist_id_get_data()` returns `NULL` for both. This happens when `g_object_run_dispose()` is called for the target of a `GBinding`, and is not an error. See https://gitlab.gnome.org/GNOME/glib/-/issues/2676 --- gobject/gobject.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index df908984b..2217e5c4b 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -5059,16 +5059,22 @@ g_weak_ref_set (GWeakRef *weak_ref, /* Remove the weak ref from the old object */ if (old_object != NULL) { + gboolean in_weak_refs_notify; + weak_locations = g_datalist_id_get_data (&old_object->qdata, quark_weak_locations); + in_weak_refs_notify = g_datalist_id_get_data (&old_object->qdata, quark_weak_refs) == NULL; /* for it to point to an object, the object must have had it added once */ - g_assert (weak_locations != NULL); + g_assert (weak_locations != NULL || in_weak_refs_notify); - *weak_locations = g_slist_remove (*weak_locations, weak_ref); - - if (!*weak_locations) + if (weak_locations != NULL) { - weak_locations_free_unlocked (weak_locations); - g_datalist_id_remove_no_notify (&old_object->qdata, quark_weak_locations); + *weak_locations = g_slist_remove (*weak_locations, weak_ref); + + if (!*weak_locations) + { + weak_locations_free_unlocked (weak_locations); + g_datalist_id_remove_no_notify (&old_object->qdata, quark_weak_locations); + } } } From 4ef2025d47770ea2da1c4cc9360f21a002d4ed62 Mon Sep 17 00:00:00 2001 From: Peter Bloomfield Date: Sun, 3 Jul 2022 15:10:31 -0400 Subject: [PATCH 11/11] gobject/tests/binding: Add a test with run-dispose Add tests in which `g_object_run_dispose()` is called on the source or target of a `GBinding`. After commit a4fa456e677246629e714d05b5de178691571b06, the target test caused a failed assertion in `g_weak_ref_set()` that was not found by the existing tests. Commit 94ba14d5424c9304342a601cb9eb0842e56d8f51 weakens the assertion to allow the test to succeed. See https://gitlab.gnome.org/GNOME/glib/-/issues/2676 --- gobject/tests/binding.c | 48 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/gobject/tests/binding.c b/gobject/tests/binding.c index b8373e345..cc6e65987 100644 --- a/gobject/tests/binding.c +++ b/gobject/tests/binding.c @@ -1089,6 +1089,52 @@ binding_concurrent_finalizing (void) } } +static void +binding_dispose_source (void) +{ + /* Test that the source can be disposed */ + BindingSource *source = g_object_new (binding_source_get_type (), NULL); + BindingTarget *target = g_object_new (binding_target_get_type (), NULL); + GBinding *binding; + + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2676"); + + binding = g_object_bind_property (source, "foo", + target, "bar", + G_BINDING_DEFAULT); + + g_object_add_weak_pointer (G_OBJECT (binding), (gpointer *) &binding); + + g_object_run_dispose (G_OBJECT (source)); + g_assert_null (binding); + + g_object_unref (target); + g_object_unref (source); +} + +static void +binding_dispose_target (void) +{ + /* Test that the target can be disposed */ + BindingSource *source = g_object_new (binding_source_get_type (), NULL); + BindingTarget *target = g_object_new (binding_target_get_type (), NULL); + GBinding *binding; + + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2676"); + + binding = g_object_bind_property (source, "foo", + target, "bar", + G_BINDING_DEFAULT); + + g_object_add_weak_pointer (G_OBJECT (binding), (gpointer *) &binding); + + g_object_run_dispose (G_OBJECT (target)); + g_assert_null (binding); + + g_object_unref (target); + g_object_unref (source); +} + int main (int argc, char *argv[]) { @@ -1111,6 +1157,8 @@ main (int argc, char *argv[]) g_test_add_func ("/binding/interface", binding_interface); g_test_add_func ("/binding/concurrent-unbind", binding_concurrent_unbind); g_test_add_func ("/binding/concurrent-finalizing", binding_concurrent_finalizing); + g_test_add_func ("/binding/dispose-source", binding_dispose_source); + g_test_add_func ("/binding/dispose-target", binding_dispose_target); return g_test_run (); }