814517 - [meta] Many prefs are read before Preferences::ResetAndReadUserPrefs() is called

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

I'm not sure how much of a problem this is but filing anyway to discuss and figure it out. While testing bug 807998 I discovered that the memory.free_dirty_pages pref added in bug 805855 is read before ResetAndReadUserPrefs is called. As a result, the value obtained is always the default value of the pref, and setting it manually via about:config has no effect.

I then set a couple of breakpoints and dumped out all the keys passed to pref_HashTableLookup before ResetAndReadUserPrefs is called, to see if there were other prefs in the same situation. Turns out there are a lot (214 unique prefs, 257 calls to pref_HashTableLookup). I haven't investigated if any of these are read again after ResetAndReadUserPrefs is called, or if anybody cares that the initial value read may not take into account what the user does in about:config, but in the case of memory.free_dirty_pages it seems to be a bug.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Properly react to changes to the 'memory.free_dirty_pages' preference

The issue introduced in bug 805855 should be fixed because the preference is being randomized during testing (see bug 805855 comment 98) and so we need to be able to read a different value from the user settings. The attached patch should fix the problem by mimicking the behavior of the rest of the AvailableMemoryTracker code regarding this kind of changes. I'm asking Justin to review it because he was the one who reviewed the original patch.

Comment 2

[Justin Lebar (not reading bugmail)]

Comment on attachment 687162 [details] [diff] [review]
Properly react to changes to the 'memory.free_dirty_pages' preference

> +#define FREE_DIRTY_PAGES_PREF "memory.free_dirty_pages"

I'm not a big fan of this sort of one-time define (even though we use it somewhat frequently in our code), but r=me either way.

Comment 3

[Justin Lebar (not reading bugmail)]

You may want to update the bug summary, if this patch is all you intend to fix in this bug.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Justin Lebar [:jlebar] from comment #2)
> > +#define FREE_DIRTY_PAGES_PREF "memory.free_dirty_pages"
> 
> I'm not a big fan of this sort of one-time define (even though we use it
> somewhat frequently in our code), but r=me either way.

Actually it's a leftover of my first try where I used it in a couple more places. It's useless, I'll get rid of it.

> You may want to update the bug summary, if this patch is all you intend to
> fix in this bug.

I'll have a look at the other instances of this problem, it shouldn't be too hard to find all the prefs consumers that are starting before ResetAndReadUserPrefs(). There's no rush in closing this after all.

Comment 5

[Justin Lebar (not reading bugmail)]

> There's no rush in closing this after all.

Well, yes and no.  We like to close bugs after a fix lands (although we don't always succeed in this respect).

If you're going to try to fix other instances of this problem, but not fix them in one fell swoop, the canonical way to do it is to have a metabug ("Many prefs are read before Preferences::ResetAndReadUserPrefs() is called") and then create a bug which blocks the metabug for each change you're going to land as a unit.

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Justin Lebar [:jlebar] from comment #5)
> If you're going to try to fix other instances of this problem, but not fix
> them in one fell swoop, the canonical way to do it is to have a metabug
> ("Many prefs are read before Preferences::ResetAndReadUserPrefs() is
> called") and then create a bug which blocks the metabug for each change
> you're going to land as a unit.

OK, I'll try to get a feeling of how many places are affected by this; if it's just a couple then I'll try to quickly fix them in this bug and be done with it. If it's more I'll open individual bugs, make this one a meta-bug and move my patch into the appropriate one.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

I finally had the time to give a good look at this one by back-tracking from every pref_HashTableLookup() call that happens before Preferences::ResetAndReadUserPrefs(). I've found at least 7 preferences of which I'm almost certain that they're not being handled properly (including the one I introduced) and another ~20 potential candidates that I need to investigate further.

Later on today I'll turn this into a meta-bug and then file separate bugs for every component I will be able to confirm is not handling its prefs correctly; I'll move the existing patch there too.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 687162 [details] [diff] [review]
Properly react to changes to the 'memory.free_dirty_pages' preference

Obsoleting as I've moved the patch to bug 850607.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

I should have filed all the instances of components reading prefs too early I could reasonably confirm  even though some of them might be deliberate choices. For those that are deliberate choices I'd still like to add a comment in the relevant piece of code to clarify that only defaults are taken into account.

There's one last instance that I haven't filed a bug for and it's jsloader.reuseGlobal being loaded in mozJSComponentLoader::ReallyInit(). There the choice of ignoring a user-set value for that preference seems deliberate considering that the code contains an assertion to ensure that the value hasn't been flipped from true to false after one or more modules have already been loaded.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Prefs loaded before user prefs are read

Just for kicks I added some logging to my local b2g build to see how much this is still happening. There's still a fair number of prefs being read before the user prefs are loaded - not sure how many of them are intentionally that way. Log attached.