GNOME Bugzilla – Bug 794805
gsettings: Fix leaks and assertion on range binding failures
Last modified: 2018-04-11 10:04:26 UTC
With glib/nautilus git master, if I go to the preferences dialog, set the "thumbnail limit" (last tab) to a value bigger than 4096, I get a runtime error in my terminal, but after closing the preferences window, this triggers an assert. This patch tries to leave the GSettings in a consistent (and non-leaking) state when such an event occurs. Since I don't know how much we want to support binding a gint property to a gsettings range, I left one issue unadressed, which is that the gobject property will be out of sync with the GSettings value, and its value will be out of range.
Created attachment 370291 [details] [review] gsettings: Fix leaks and assertion on range binding failures When using g_settings_bind(), if a range binding triggers a range check failure, g_settings_binding_property_changed() will return early, but it won't cleanup properly causing some leaks. The binding will also still be marked as 'running', which causes an assertion failure when trying to free it: "g_settings_binding_free: assertion failed: (!binding->running)" Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Review of attachment 370291 [details] [review]: ::: gio/gsettings.c @@ +2722,3 @@ binding->key.name, g_settings_schema_get_id (binding->key.schema), + variant_str); + g_free(variant_str); Nitpick: Missing space before `(`. @@ +2730,3 @@ g_variant_unref (variant); } +end: Instead of introducing a `goto`, how about: gboolean valid = TRUE; g_variant_take_ref (variant); if (!g_settings_schema_key_type_check (…)) { g_critical (…); valid = FALSE; } if (valid && !g_settings_schema_key_range_check (…)) { g_critical (…); valid = FALSE; } if (valid) g_settings_write_to_backend (…); g_variant_unref (variant); ::: gio/tests/gsettings.c @@ +1325,3 @@ g_assert_cmpint (i, ==, TEST_FLAGS_MOURNING | TEST_FLAGS_WALKING); + g_settings_bind (settings, "uint", obj, "uint", G_SETTINGS_BIND_DEFAULT); How about adding another few tests to cover the first g_critical() branch, i.e. incorrect types? @@ +1336,3 @@ + + g_settings_bind (settings, "range", obj, "uint", G_SETTINGS_BIND_DEFAULT); + g_object_set(obj, "uint", 22, NULL); Nitpick: Missing space before `(`. @@ +1344,3 @@ + g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL, + "* is out of schema-specified range for*"); + g_object_set(obj, "uint", 45, NULL); Nitpick: Missing space before `(`.
Created attachment 370772 [details] [review] gsettings: Fix leaks and assertion on range binding failures When using g_settings_bind(), if a range binding triggers a range check failure, g_settings_binding_property_changed() will return early, but it won't cleanup properly causing some leaks. The binding will also still be marked as 'running', which causes an assertion failure when trying to free it: "g_settings_binding_free: assertion failed: (!binding->running)" Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Review of attachment 370291 [details] [review]: ::: gio/gsettings.c @@ +2730,3 @@ g_variant_unref (variant); } +end: I think I prefer the goto, but your approach allows to avoid repeating the 'g_variant_unref()' multiple times, I've changed it. ::: gio/tests/gsettings.c @@ +1325,3 @@ g_assert_cmpint (i, ==, TEST_FLAGS_MOURNING | TEST_FLAGS_WALKING); + g_settings_bind (settings, "uint", obj, "uint", G_SETTINGS_BIND_DEFAULT); I added one, let me know if more are needed. @@ +1336,3 @@ + + g_settings_bind (settings, "range", obj, "uint", G_SETTINGS_BIND_DEFAULT); + g_object_set(obj, "uint", 22, NULL); Ah sorry, I'm bad at that :( Should be all fixed now.
Review of attachment 370772 [details] [review]: Lovely, thanks! I’ll do a bit of local testing and then push it to master.
Attachment 370772 [details] pushed as fbbad52 - gsettings: Fix leaks and assertion on range binding failures