Created attachment 364083 [details]
C Stacktrace

This occurrence happenned on idle in a normal session (no valgrind)

*** Bug 721590 has been marked as a duplicate of this bug. ***

After poking around a little bit I found that this occurs as a result of dconf_engine_watch_established() calling dconf_engine_change_notify() with the root path "/" as the prefix in dconf-engine.c. There is a comment there which reads:

/* Our recorded state does not match the current state.  Something
 * must have changed while our watch requests were on the wire.
 *
 * We don't know what changed, so we can just say that potentially
 * everything changed.  This case is very rare, anyway...
 */

The state variable is incremented in dconf_engine_acquire_sources(). This appears to happen once on startup during some kind of initialisation phase (which does not result in any spurious changed signals), but then sometimes a second time. The second time seems to result in changed events being emitted for all keys.

Some more information:

The reason the state variable is incremented is that the following call stack happens: 

dconf_engine_acquire_sources()
dconf_engine_source_refresh()
dconf_engine_source_user_needs_reopen()
dconf_shm_is_flagged()

dconf_shm_is_flagged() returns TRUE, because another process has called dconf_shm_flag() to indicate that it has overwritten the settings file. This happens in between a call to dconf_engine_watch fast() (which returns optimistically), and dconf_engine_watch_established() (which indicates that the subscription is actually working). In this case, the dconf gsettings backend does not know whether the setting being watched has changed and therefore whether it should emit a changed event for it. It also has no record of which setting was being subscribed to, so it emits a changed event for everything.

The process which calls dconf_shm_flag() seems to be dconf-service, which runs automatically when applications change dconf settings over DBus. One such example is the gnome shell extensions page (when used in chromium with with chrome gnome shell) https://extensions.gnome.org/local/. Turning an extension on or off results in dconf_shm_flag() being called and then the gnome-shell process reaching the state where it sends a changed event for everything.

This is a stacktrace of the dconf-service process where it crashed at my breakpoint:

+ Trace 238189

I think a simple solution for the majority of the problem would be to make the OutstandingWatch which keeps the state of pending watch operations store the key that is being subscribed to. Then if this unfortunate sequence of events occurs, at least it will only be necessary to emit a (possibly spurious) changed event for the setting that was actually being subscribed to, rather than all settings.

Created attachment 365364 [details] [review]
dconf engine: maintain a list of watched and pending paths, and avoid emitting defensive change notifications when a subscription was already active

I tried this strategy (restrict the change notification to only the path being subscribed to) but it did not solve the problem of infinite loops.

So additionally, I made the dconf engine store a set of paths that it is currently subscribed to (separate sets for when a subscription is being established and when it has been established).

This means that although there can still be spurious change notifications for a large set of keys in the case where a setting is changed while a watch request is in progress, this is no longer the case if a watch had already been previously established.

When applied with the patches in bug 789639, this solves the infinite loops I was experiencing in gnome-shell, and I can run it reliably in valgrind with all my shell extensions.

Review of attachment 365364 [details] [review]:

the idea of preserving the knowledge about watched paths makes sense to me

Review of attachment 365364 [details] [review]:

::: engine/dconf-engine.c
@@ +839,3 @@
     }
 
+  dconf_engine_set_watching(engine, ow->path, TRUE, TRUE);

Coding style trivia: space before (, throughout

@@ +905,3 @@
                                          dconf_engine_make_match_rule (engine->sources[i], path), NULL, NULL);
+
+  dconf_engine_set_watching(engine, g_strdup(path), FALSE, FALSE);

The memory handling of dconf_engine_set_watching seems dubious to me: it supposedly takes a const char *, yet you strdup here, and the function actually takes ownership and sometimes frees the passed string. I think this would be
much cleaner if dconf_engine_set_watching was doing the strdup itself

Created attachment 366936 [details] [review]
dconf engine: maintain a list of watched and pending paths, and avoid emitting defensive change notifications when a subscription was already active

Thanks, I've updated the patch

 - function calls now have a space between the function name and parentheses
 - dconf_engine_set_watching treats its path argument as a const, and calls g_strdup to make a copy if it needs to keep it after invocation.
 - the tests pass
 - there are tests for new functions

Review of attachment 366936 [details] [review]:

If you fix these, I think it is good to commit.

::: engine/dconf-engine.c
@@ +847,3 @@
                          const gchar *path)
 {
+  if (dconf_engine_is_watching(engine, path, TRUE))

Still a few spaces missing

@@ +1412,3 @@
+
+void
+dconf_engine_set_watching (DConfEngine *engine, const gchar *path, const gboolean is_watching, const gboolean is_established)

Not entirely sure about dconf coding style, but typically, we align the parameters, one per line.

@@ +1413,3 @@
+void
+dconf_engine_set_watching (DConfEngine *engine, const gchar *path, const gboolean is_watching, const gboolean is_established)
+  {

the function level block should be at column 0

@@ +1436,3 @@
+gboolean
+dconf_engine_is_watching (DConfEngine *engine, const gchar *path, const gboolean only_established)
+  {

Same here

Created attachment 368355 [details] [review]
dconf engine: maintain a list of watched and pending paths, and avoid emitting defensive change notifications when a subscription was already active

Thanks, I've fixed those style issues and a few others along the same lines. Should be ready :)

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/dconf/issues/41.