1093302 - nsIPrefService should have an attribute exposing whether user prefs have been read

Status: REOPENED

(Core :: Preferences: Backend, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Bug 1092808 comment 4 shows an example of a case where a preference observer is registered early enough in the startup process that it gets fired when we subsequently load user preferences.

Shouldn't preference observer notifications be suppressed until the preferences system has finished initializing itself (i.e. reading all default and user prefs)?

Comment 1

[Benjamin Smedberg]

No, definitely not.

Before we load the profile, Get*Pref will reflect the default value. After the load, it will be the user pref. So if code gets the pref value before we load the profile, we want to fire the notification that the pref value changed (and much code in gecko relies on that behavior).

Comment 2

[:Gavin Sharp [email: [e-mail]]]

That's not consistent with user expectations for this API, so we should mitigate it somehow. Pretty sure most pref API users don't expect this behavior.

It's also a difficult issue to work around because as far as I know you can't know whether we're before or after the "user prefs have been loaded" point without observing other somewhat unrelated notifications.

How about requiring opt-in to be notified of these changes? E.g. via an extra "I confirm that I am prepared to handle changes from loading user pref files" argument to addObserver that is checked before we fire these notifications?

Comment 3

[Justin Dolske [:Dolske]]

The current behavior makes sense to me in general, although I can see how it's a potential footgun for things like bug 1092808. But having an opt-in could just as well be a footgun for something that wants to handle this case. I think 1092808 is just an oddball: most things want to know what a pref currently is, and setup observers to change behavior when the pref changes. 1092808 only wants to do things when a pref _changes_ -- it doesn't care what the initial state is.

It might be interesting for a pref observer to know what caused a pref to change, but that wouldn't actually help here.

Comment 4

[Benjamin Smedberg]

Indeed. I don't quite understand what bug 1092808 is about, but the purpose of this API is to allow code to change runtime behavior based on the current value of a pref. That covers all sorts of potential causes for change, which may include:

* loading the profile
* installing/enabling/disabling a restartless addon
* code calling set*Pref from user action
* syncing prefs (?)

It's true that there is currently not a good marker to tell code what the current startup sequence is. I'd love to fix that, but it's not likely to get to the top of the priority list any time soon.
 
I still think that this is WONTFIX.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Indeed. I don't quite understand what bug 1092808 is about, but the purpose
> of this API is to allow code to change runtime behavior based on the current
> value of a pref.

For many (if not most) pref API callers, the concept "before profile is loaded" pref state is not useful (because e.g. they never expect to run that early in the startup process). This is true of the vast majority of pref API uses in the front-end. Having them have to deal with changes due to loading of user pref files from the profile is thus confusing and can lead to unexpected behavior.

Clearly there are callers of this prefs API for whom that distinction matters, but I suspect they're not as common (I'm biased), and I don't think we should be catering to both of these categories of users with the same API.

Comment 6

[Benjamin Smedberg]

Basically all of gecko code that is using this is loaded before the profile runs. That's the common case that we need to preserve.

If this is a request for a new kind of API that only notifies on specific conditions, we need to be specific about the conditions we're excluding, and also whether the additional complexity is worth it since the prefs code really needs a rewrite if we're going to be tackling API issues.

Comment 7

[Justin Dolske [:Dolske]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #5)

> For many (if not most) pref API callers, the concept "before profile is
> loaded" pref state is not useful (because e.g. they never expect to run that
> early in the startup process).

And yet that's exactly when the bug 1092808 code was running. :) It's not hard imagine a situation as follows in some future bug:

0) We fix this bug, so the read of user.js no longer notifies observers
1) Hypothetical future code added that reads a pref and sets an observer too early in startup
2) user.js is read, which includes a non-default value to the pref in #1
3) Something breaks and a bug report is filed because the user's pref is ignored.

I think the core of the issue it that it's easy to forget there are places code can run before a profile is in use. Not sure there's a good fix for that:

 * Add comments and docs, and hope people notice/remember
 * Change code like nsBrowserGlue to only initialize after profile-after-change
 * Change prefs API to make it harder to footgun yourself -- e.g. have getBranch() throw early on,
   and force explicitly using getDefaultBranch()

[That last point makes me wonder what happens if a pref is set by code before we read user.js --
will user.js data clobber existing values, or will existing values clobber user.js? I wonder if there's any code that does this and expects the wrong behavior...]

Comment 8

[:Gijs (away/busy until Mar 10)]

It sounds like as a first step, we should at least expose whether the initial read has taken place and allow code to hook into that happening to post-set prefs if required? (I'm assuming the read takes precedence over things set while no read has yet taken place, because that seems like what the simplest implementation would be doing - but I've not checked!)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Justin Dolske [:Dolske] from comment #7)
> > For many (if not most) pref API callers, the concept "before profile is
> > loaded" pref state is not useful (because e.g. they never expect to run that
> > early in the startup process).
> 
> And yet that's exactly when the bug 1092808 code was running. :)

Unintentionally. If there was a simplified pref API that didn't expose this distinction, it wouldn't have been a problem.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Basically all of gecko code that is using this is loaded before the profile
> runs. That's the common case that we need to preserve.

I don't buy that this affects "basically all gecko code [that uses the pref API]". Plenty of Gecko pref observers are added lazily long after profile selection. I just instrumented a Firefox build to log when user prefs are read, and when observer are added. On a relatively clean startup+load webpage+quit workload, there are 159 calls to AddObserver prior to Preferences::ReadUserPrefs, and 443 after. Those 443 are spread across Gecko and the front-end, but they are all examples of consumers who definitely don't care about the pre-profile state.

I agree that reworking/splitting the prefs API is a huge effort that we're not going to embark on just to solve this problem, but we should ideally be on the same page about what the common use cases and expectations are for this API.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Let's morph this to something concrete that would allow mitigating this problem in  front-end preference wrappers like Prefences.jsm.