"once" StaticPrefs anti-footgun is a footgun
Categories
(Core :: Preferences: Backend, defect)
Tracking
()
People
(Reporter: tcampbell, Unassigned)
Details
https://bugzilla.mozilla.org/show_bug.cgi?id=1697904#c14
Currently StaticPrefs::InitOncePrefs
is called on first use of a static pref, at which point it setups up the "once" mirrors and in debug builds will snapshot the pref state to detect errors. This being dynamic (eg at first use) can lead to surprises when something causes it to happen at a different point in startup.
The specific issue I ran into was trying to use "mirror: once" prefs for SpiderMonkey which runs early in startup. The EnterprisePolicies logic runs slightly after this (since it is coded in JS) and mutates prefs such as layers.acceleration.disabled
and that trips things up. (The browser/components/enterprisepolicies
tests caught this).
I'm not entirely sure what to classify as the root cause here and there are a few different things we might consider:
- Have
StaticPrefs::InitOncePrefs
be an explicit phase of startup rather thanMaybeInitOncePrefs
. This would make things more predictable and catch bugs sooner. Eg if a subsystem before then use such a pref, we'd fail on a nullptr assertion. - Mark prefs manipulated by EnterprisePolicies as "mirror: always". This probably doesn't make GFX happy though.
- Ban spidermonkey from using "mirror: once" directly and continue to use LoadStartupJSPrefs as a bespoke form of "mirror: once" for SpiderMonkey (which really does want "once" prefs because changing dynamic code-generation parameters on a running JIT is quite scary).
Here is the point that EnterprisePolicy runs JS to mutate prefs that are supposed to be "once": https://searchfox.org/mozilla-central/rev/6806b58cd25a88aad78aa3dda9c3f3aa75278d78/toolkit/xre/nsXREDirProvider.cpp#973
Slightly concerning is the EnterprisePolicies:Restart
observer that resets prefs during tests but after startup.
Anyone have ideas about what is best to do here? There don't seem to be many safety rails about what prefs can be mutated by EnterprisePolicies vs what might be activated in order to run the policy code.
Reporter | ||
Comment 1•4 years ago
|
||
Another possibility might be to break down "mirror: once" into different phases to split JS from GFX into different categories. This would probably be paired with removing MaybeInitOncePrefs in favour of an explicit step in startup process. Something like "early_once" vs "once", where the "early_once" prefs cannot be modified after they come from pref file / parent-process shmem.
Reporter | ||
Comment 2•4 years ago
|
||
https://searchfox.org/mozilla-central/rev/898cc2d20f2ceb2723e708162a98653a04b32496/toolkit/mozapps/update/UpdateService.jsm#2703-2708
I guess reload-default-prefs
throws a bit of complexity into prefs and brings a lot of history with it..
For SpiderMonkey prefs, I think the appropriate answer rather than tackling the entire startup pref system at once is as follows:
- Use
mirror: always
for SpiderMonkey restart-required prefs - Use
do_not_use_directly: true // LoadStartupJSPrefs
for SpiderMonkey restart-required prefs - Add a comment to the
javascript.
section of the prefs YAML to explain this - Go with Bug 1698102 and build our own
js::StaticPrefs
mirrors by parsing the original prefs YAML LoadStartupJSPrefs
would be automated and copymozilla::StaticPrefs::javascript_options_XXX_DoNotUseDirectly()
tojs::StaticPrefs::javascript_options_XXX()
For the existing once
mirror system, I still wonder if we are better off being explicit about when they setup rather than "on first use".
Comment 3•4 years ago
|
||
Actually, despite the tests passing, the policy that uses layers.acceleration.disabled doesn't work - it sets it too late during startup.
Because the test uses reload, it appears to work.
What I'd actually love to figure out is if there is some way we can set policy preferences earlier in startup so we can solve these (and other problems).
Comment 4•4 years ago
|
||
I've looked at this in the past for some other prefs and it'd be a difficult thing to fix. Preferences service cannot fully init until a certain stage in startup, and silently fails if it cannot be reached. It's possible splitting preferences startup into multiple stages and explicitly loading anything we can as early as possible (like early_once
mentioned above) can help but that all depends on working out a way to determine what is safe to early-load that can't be an always mirror and how much we can init.
For the time being at least I think it's better to load in some do-not-use-directly
always prefs as mentioned above. While these are prefs that should behave like once
prefs, MaybeInitOncePrefs
simply doesn't work until we are far enough into startup. I should add a note to our prefs docs about this in StaticPrefList.yaml
since the existing note about once
-prefs might be misleading about how they can be used.
Reporter | ||
Comment 5•4 years ago
|
||
Sounds like staying away from the current "mirror: once" system makes sense for now since SpiderMonkey can continue to roll its own. I still would like to parse the YAML file from within SpiderMonkey to generate the shadow mirrors automagically.
Kris, what is your general reaction to adding another optional field for the YAML properties. Some sort of spidermonkey_mirror: once/always
? I would use this in my own mirror generator script to produce the LoadStartupJSPrefs
/ ReloadPrefsCallback
bodies automatically. Right now I have I'm prototyping with a dirty hack that I mirror if prefix is javascript.options.
and check the existing do-no-use-directly
to determine if it is spidermonkey-once.
Comment 6•4 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #5)
Kris, what is your general reaction to adding another optional field for the YAML properties. Some sort of
spidermonkey_mirror: once/always
? I would use this in my own mirror generator script to produce theLoadStartupJSPrefs
/ReloadPrefsCallback
bodies automatically.
Which prefs need to be handled in ReloadPrefsCallback
? I was hoping we could move all JS prefs to be process-wide and handled in LoadStartupJSPrefs
.
Comment 7•3 years ago
|
||
Adding needinfo for the question on comment 5.
Comment 8•3 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #5)
Kris, what is your general reaction to adding another optional field for the YAML properties. Some sort of
spidermonkey_mirror: once/always
? I would use this in my own mirror generator script to produce theLoadStartupJSPrefs
/ReloadPrefsCallback
bodies automatically. Right now I have I'm prototyping with a dirty hack that I mirror if prefix isjavascript.options.
and check the existingdo-no-use-directly
to determine if it is spidermonkey-once.
My general feelings about this are that if we want to add some spidermonkey specific mirror property then we will also want to implement some kind of lint check to avoid confusion. I'm fine with adding this, but we will need to ensure via checks that anything with the spidermonkey_mirror
field must be mirrored and used in spidermonkey and not just randomly added with this field due to confusion about how mirroring works.
Description
•