Open Bug 1698311 Opened 3 years ago Updated 3 years ago

"once" StaticPrefs anti-footgun is a footgun

Categories

(Core :: Preferences: Backend, defect)

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 than MaybeInitOncePrefs. 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.

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.

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 copy mozilla::StaticPrefs::javascript_options_XXX_DoNotUseDirectly() to js::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".

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).

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.

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.

(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 the LoadStartupJSPrefs / 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.

Adding needinfo for the question on comment 5.

Severity: -- → S4
Flags: needinfo?(kwright)

(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 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.

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.

Flags: needinfo?(kwright)
You need to log in before you can comment on or make changes to this bug.