1477254 - Value mismatch when reading preferences with e10s on

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Jean-Yves Avenard [:jya]]

I was investigating the reasons behind:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60af9520abe42ab482485a8b75bf7b445b164aa5

I noticed something that definitely didn't feel right.

In all.js I added:
#ifdef MOZ_AV1
pref("media.av1.enabled", false);
#endif

Is StaticPrefs we have:
https://searchfox.org/mozilla-central/source/modules/libpref/init/StaticPrefList.h#867


now in the code, main thread, I call:
    fprintf(stderr, "StaticPrefs::MediaAv1Enabled()=%d\n", bool(StaticPrefs::MediaAv1Enabled()));

In a mochitest, I exercise this code:
./mach mochitest --disable-e10s --run-until-failure dom/media/test/test_can_play_type_webm.html

I read:
 0:28.73 GECKO(6897) StaticPrefs::MediaAv1Enabled()=0

so far so good.

no running:
./mach mochitest --run-until-failure dom/media/test/test_can_play_type_webm.html
(note that e10s isn't disabled)

first run I read:
 0:28.90 GECKO(6956) StaticPrefs::MediaAv1Enabled()=1

It reads true, seems that the default value set in all.js doesn't override the default value set in StaticPrefs

This is a regression in behaviour from the earlier preferences engine.

Note that even with e10s off, it will end up failing reading that pref as true from time to time.

Comment 1

[Jean-Yves Avenard [:jya]]

I suspect bug 1471025 to be the reason behind this problem

Comment 2

[Kris Maglione [:kmag]]

This is actually expected, and intentionally unsupported:

https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/modules/libpref/Preferences.cpp#4791-4800

If you need to change a default pref at build time, please change it in StaticPrefList.h. If you need to change it at runtime, please change its user value.

Comment 3

[Jean-Yves Avenard [:jya]]

well, then there's a fair amount of pref value to be changed to all.js

Because typically, when a feature is enabled, it's only enabled in all.js and the StaticPrefs isn't touched

Comment 4

[Jean-Yves Avenard [:jya]]

And when there's a mismatch, an assertion should be run IMHO, as potentially this could cause serious security issue (like in bug 1477253) where we have code with known security bugs behind a pref.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Because typically, when a feature is enabled, it's only enabled in all.js
> and the StaticPrefs isn't touched

Why? That doesn't sound right at all.

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> And when there's a mismatch, an assertion should be run IMHO

It probably should, yes.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Kris Maglione [:kmag] from comment #5)
 
> Why? That doesn't sound right at all.

just my experience over the years looking at when a feature is turned on...

Now, having said that bug 1448222 (and others) removed a lot of the prefs definition in all.js so the problem may have been minimized.

But with just one example causing a sec-high today, someone needs to go over all the default values of the pref and ensure there's no discrepency.
The behaviour of the prefs having change, we need to double check.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

From what I recall, the idea of disallowing overrides on static prefs was attempted at bug 1436655, but ultimately rolled back due to concerns with re-packages and live pref flips on release (see comment 43 there for the conclusion)

Comment 8

[Kris Maglione [:kmag]]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #5)
> > Why? That doesn't sound right at all.
> 
> just my experience over the years looking at when a feature is turned on...

But StaticPrefs hasn't existed for years. It's only existed for a few months.

Comment 9

[Jean-Yves Avenard [:jya]]

Felipe point is a very important one. This change will make hotfix unusable. I had forgotten about that one.
Having override allowed is certainly important.

 Most Linux distribution repackaging things will play with prefs in all.js too.

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> Felipe point is a very important one. This change will make hotfix unusable.
> I had forgotten about that one.
> Having override allowed is certainly important.
> 
>  Most Linux distribution repackaging things will play with prefs in all.js
> too.

Thats fine, but why would there be a usage change? StsticPrefs was almost a drop in replacement. Most devs would be unfamiliar with those changes, and that you can no longer override prefs.

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to Kris Maglione [:kmag] from comment #8)
> (In reply to Jean-Yves Avenard [:jya] from comment #6)
> > (In reply to Kris Maglione [:kmag] from comment #5)
> > > Why? That doesn't sound right at all.
> > 
> > just my experience over the years looking at when a feature is turned on...
> 
> But StaticPrefs hasn't existed for years. It's only existed for a few months.

In the message above, i was referring to MediaPrefs and others that got replaced by StaticPrefs

Comment 12

[Kris Maglione [:kmag]]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> Felipe point is a very important one. This change will make hotfix unusable.
> I had forgotten about that one.
> Having override allowed is certainly important.

This doesn't affect the hotfix. It only applies to unlocked varcache preferences without user values. Hotfixes change preference user values.

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> Thats fine, but why would there be a usage change?

Because of Project Fission.

Under Fission, there are sites that require us to immediately spawn 30 content processes. At the moment, on Windows 10, it takes us about 150ms to start up a content process. That's 4.5 seconds of CPU time.

We need to reduce that number as much as possible, and every millisecond counts. The only way for us to handle arbitrary default value changes for all varcache prefs at the moment is to look up every one at content process startup. That could take multiple milliseconds. Or, 2ms * 30 processes = 60ms CPU time. That's not trivial, and it's not something I think we can afford without serious justification.

> Most devs would be unfamiliar with those changes, and that you can no longer override prefs.

You can still override prefs by changing the user value. If the default value needs to be changed, it should be changed in StaticPrefList, not all.js.

As far as I'm concerned, changing a value in all.js for a preference defined in StaticPrefs is a bug. It should probably be handled with an assertion.

Comment 13

[Jean-Yves Avenard [:jya]]

Something else, likely related (I just got backed out for it).

So the default value of media.av1.enabled now default to false.

The mochitest test the code like so:

  await SpecialPowers.pushPrefEnv({"set": [["media.av1.enabled", true]]});
  check("video/webm; codecs=\"av1\"", "probably");

  await SpecialPowers.pushPrefEnv({"set": [["media.av1.enabled", false]]});
  check("video/webm; codecs=\"av1\"", "");


I get an intermittent failure on the first test.
On Windows 7 that's a permafail, okay on mac and linux.
So it's no longer possible to use pushPrefEnv either, the result isn't guaranteed?

Comment 14

[Jean-Yves Avenard [:jya]]

Never mind, I think I know what the problem is..

Comment 15

[Kris Maglione [:kmag]]

(In reply to Jean-Yves Avenard [:jya] from comment #13)
> So it's no longer possible to use pushPrefEnv either, the result isn't
> guaranteed?

pushPrefEnv should work exactly the same as it did before.

Comment 16

[Kris Maglione [:kmag]]

Attachment: Bug 1477254: Assert that varcache prefs match pref values at content process startup.

In order to avoid the overhead of doing a full pref lookup for every static
var cache at content process startup, we currently assume that the default
value of any static varcache pref will always match the default value of its
database entry (as long as the pref isn't locked). This lets us only perform
lookups for preferences which have a user value, or are locked.

If the default values of those preferences are changed in a bundled preference
file, though, the varcache value will be correct in the parent process, but
not in child processes. Since this is an easy mistake to make, we should
assert that it doesn't happen.

Note: This change only affects applications which use e10s. Applications like
Thunderbird can still override default values of any static pref with
impunity. Repacks and distributors can only do so by changing user values or
locking the preference after the change (which is the standard practice for
enterprise deployments).

Review commit: https://reviewboard.mozilla.org/r/258474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258474/

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8993776 [details]
Bug 1477254: Assert that varcache prefs match pref values at content process startup.

https://reviewboard.mozilla.org/r/258474/#review265588

I'm giving r+ for this patch because it improves upon the current situation -- it'll identify some cases where previously we'd just silently get unexpected behaviour.

But I'm not satisfied with the current behaviour in general: "overrides in all.js work except when they don't, and the reasons are multi-factored and subtle". Is there a way to get back the old behaviour? As Felipe noted in comment 7, I tried disallowing overrides when introducing StaticPrefList but backed off. Prefs get used in all sorts of strange ways, unfortunately.

Apologies for overlooking this behaviour change when the big shared-memory change landed.

::: modules/libpref/Preferences.cpp:4846
(Diff revision 1)
>          NotifyCallbacks(pref.Name(), PrefWrapper(pref));
>        }
>      }
>  
> +#ifdef DEBUG
> +    // Check that all varcache preferences match their current values.

This comment could be slightly more expansive, e.g. explaining the circumstances in which this assertion can fail, and how to fix it.

::: modules/libpref/Preferences.cpp:4850
(Diff revision 1)
> +#ifdef DEBUG
> +    // Check that all varcache preferences match their current values.
> +
> +#define PREF(name, cpp_type, value)
> +#define VARCACHE_PREF(name, id, cpp_type, value)                               \
> +  MOZ_ASSERT(GetPref<StripAtomic<cpp_type>>(name, value) == StaticPrefs::id(), \

Would a MOZ_RELEASE_ASSERT be reasonable here? It would catch more violations.

Comment 18

[Kris Maglione [:kmag]]

(In reply to Nicholas Nethercote [:njn] from comment #17)
> But I'm not satisfied with the current behaviour in general: "overrides in
> all.js work except when they don't, and the reasons are multi-factored and
> subtle". Is there a way to get back the old behaviour? As Felipe noted in
> comment 7, I tried disallowing overrides when introducing StaticPrefList but
> backed off. Prefs get used in all sorts of strange ways, unfortunately.

I'm still thinking about it. I'm leaning towards adding another flag for prefs with default values which have changed more than once. That will always wind up true for static prefs that get redefined by some other code. For non-static prefs, it won't matter, since this issue only affects static pref var caches.

> ::: modules/libpref/Preferences.cpp:4850
> (Diff revision 1)
> > +#ifdef DEBUG
> > +    // Check that all varcache preferences match their current values.
> > +
> > +#define PREF(name, cpp_type, value)
> > +#define VARCACHE_PREF(name, id, cpp_type, value)                               \
> > +  MOZ_ASSERT(GetPref<StripAtomic<cpp_type>>(name, value) == StaticPrefs::id(), \
> 
> Would a MOZ_RELEASE_ASSERT be reasonable here? It would catch more
> violations.

I don't think we can afford it. We may as well just reset the values of all of the pref caches at this point.

Comment 19

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b389b00030ed38cdb3543aa5d1a67795be47565
Bug 1477254: Assert that varcache prefs match pref values at content process startup. r=njn

Comment 20

[Stefan Hindli [:stefan_hindli]]

Backed out 2 changesets (bug 1477254, bug 1475836) for bustages in builds/worker/workspace/build/src/modules/libpref/Preferences.cpp:4791:1 on a CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=189678167
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189678157&repo=mozilla-inbound&lineNumber=11432

[task 2018-07-24T00:47:47.237Z] 00:47:47     INFO -  /builds/worker/workspace/build/src/modules/libpref/Preferences.cpp:4791:1: error: unused function 'GetPref' [-Werror,-Wunused-function]
[task 2018-07-24T00:47:47.238Z] 00:47:47     INFO -  GetPref<bool>(const char* aName, bool aDefaultValue)
[task 2018-07-24T00:47:47.239Z] 00:47:47     INFO -  ^
[task 2018-07-24T00:47:47.240Z] 00:47:47     INFO -  /builds/worker/workspace/build/src/modules/libpref/Preferences.cpp:4798:1: error: unused function 'GetPref' [-Werror,-Wunused-function]
[task 2018-07-24T00:47:47.240Z] 00:47:47     INFO -  GetPref<int32_t>(const char* aName, int32_t aDefaultValue)
[task 2018-07-24T00:47:47.241Z] 00:47:47     INFO -  ^
[task 2018-07-24T00:47:47.241Z] 00:47:47     INFO -  /builds/worker/workspace/build/src/modules/libpref/Preferences.cpp:4805:1: error: unused function 'GetPref' [-Werror,-Wunused-function]
[task 2018-07-24T00:47:47.242Z] 00:47:47     INFO -  GetPref<uint32_t>(const char* aName, uint32_t aDefaultValue)
[task 2018-07-24T00:47:47.242Z] 00:47:47     INFO -  ^
[task 2018-07-24T00:47:47.244Z] 00:47:47     INFO -  3 errors generated.
[task 2018-07-24T00:47:47.244Z] 00:47:47     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1052: recipe for target 'Unified_cpp_modules_libpref0.o' failed
[task 2018-07-24T00:47:47.245Z] 00:47:47     INFO -  make[4]: *** [Unified_cpp_modules_libpref0.o] Error 1
[task 2018-07-24T00:47:47.249Z] 00:47:47     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/modules/libpref'
[task 2018-07-24T00:47:47.249Z] 00:47:47     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'modules/libpref/target' failed
[task 2018-07-24T00:47:47.250Z] 00:47:47     INFO -  make[3]: *** [modules/libpref/target] Error 2
[task 2018-07-24T00:47:47.250Z] 00:47:47     INFO -  make[3]: *** Waiting for unfinished jobs....

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/232216b44fce
Backed out 2 changesets (bug 1477254, bug 1475836) for bustages in builds/worker/workspace/build/src/modules/libpref/Preferences.cpp:4791:1 on a CLOSED TREE

Comment 22

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d68c6c56856cdd0306fc7c3491438d301526db51
Bug 1477254: Assert that varcache prefs match pref values at content process startup. r=njn

Comment 23

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d68c6c56856c