Closed Bug 1477254 Opened 4 years ago Closed 4 years ago

Value mismatch when reading preferences with e10s on

Categories

(Core :: Preferences: Backend, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed

People

(Reporter: jya, Assigned: kmag)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Keywords: regression
I suspect bug 1471025 to be the reason behind this problem
Blocks: 1471025
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.
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
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.
(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.
(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.
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)
(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.
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.
(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.
(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
(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.
Flags: needinfo?(kmaglione+bmo)
(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.
See Also: → 1477364
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.
Attachment #8993776 - Flags: review?(n.nethercote) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b389b00030ed38cdb3543aa5d1a67795be47565
Bug 1477254: Assert that varcache prefs match pref values at content process startup. r=njn
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....
Flags: needinfo?(n.nethercote)
Flags: needinfo?(n.nethercote) → needinfo?(kmaglione+bmo)
Backout by shindli@mozilla.com:
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/d68c6c56856cdd0306fc7c3491438d301526db51
Bug 1477254: Assert that varcache prefs match pref values at content process startup. r=njn
See Also: → 1477904
https://hg.mozilla.org/mozilla-central/rev/d68c6c56856c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee: nobody → kmaglione+bmo
Depends on: 1478065
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.