After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 794805 - gsettings: Fix leaks and assertion on range binding failures
gsettings: Fix leaks and assertion on range binding failures
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2018-03-29 12:21 UTC by Christophe Fergeau
Modified: 2018-04-11 10:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gsettings: Fix leaks and assertion on range binding failures (4.55 KB, patch)
2018-03-29 12:21 UTC, Christophe Fergeau
none Details | Review
gsettings: Fix leaks and assertion on range binding failures (6.10 KB, patch)
2018-04-11 07:44 UTC, Christophe Fergeau
committed Details | Review

Description Christophe Fergeau 2018-03-29 12:21:27 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.
Comment 1 Christophe Fergeau 2018-03-29 12:21:33 UTC
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>
Comment 2 Philip Withnall 2018-04-10 10:38:01 UTC
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 `(`.
Comment 3 Christophe Fergeau 2018-04-11 07:44:48 UTC
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>
Comment 4 Christophe Fergeau 2018-04-11 07:48:16 UTC
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.
Comment 5 Philip Withnall 2018-04-11 09:42:07 UTC
Review of attachment 370772 [details] [review]:

Lovely, thanks! I’ll do a bit of local testing and then push it to master.
Comment 6 Philip Withnall 2018-04-11 10:04:20 UTC
Attachment 370772 [details] pushed as fbbad52 - gsettings: Fix leaks and assertion on range binding failures