1600390 - Cache the values of the CSP prefs in the ExtensionPolicyService

Status: RESOLVED FIXED

(WebExtensions :: General, task)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

In the patch for bug 1581611, a whitelist entry was added to browser_preferences_usage.js startup performance test to allow the default CSP pref to be accessed up to 50 times at startup.

In bug 1598223, this is being bumped to 51.

We should cache the values of the CSP prefs in ExtensionPolicyService to eliminate the need for this whitelist entry. This was discussed in the patch, but not implemented. We can't use static prefs for this since String values cannot be mirrored.

I'm not sure if there is an idiom equivalent to XPCOMUtils.defineLazyPreferenceGetter in C++ so this might mean adding a pref observer and manually keeping the value up to date.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1600390 - Cache the values of the CSP prefs in the ExtensionPolicyService. r=mconley,mixedpuppy

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91bd24b26a94314754fd19b1cc77b5213a68ffe6

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Shane, Mike, Gijs: The three of you were discussing making these Static Prefs or caching them in the patch that added them.

It seems like it was unclear at the time whether these needed to be kept updated at runtime. My patch does keep them updated via a pref observer, but I was wondering:

1. Do we want to support this? Can we omit the observer? This class is instantiated in parent and child processes, so there's a chance the values will go out of sync if a child is created after the value changes. I don't like that, and I also don't feel like making children query the parent for the value, really silly stuff and the observer is better.

2. Why do these need to be prefs in the first place? Do we expect users to change them? Is it for enterprise configurability? Just want to understand the situation.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

FWIW, it seems like there's some legacy here, the pref was added in 2016. I'd rather not mess around too much...

Comment 5

[Shane Caraveo (:mixedpuppy)]

IMO we can omit the observer. As I commented in phab, I'd prefer these values to be locked down. The only place I might consider wanting to change them would be in tests.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)

IMO we can omit the observer. As I commented in phab, I'd prefer these values to be locked down. The only place I might consider wanting to change them would be in tests.

...or in enterprise policy

Comment 7

[:Gijs (he/him)]

Manual observer-ing seems fine to me given the parent/child dissonance problem you describe. If we want to prevent runtime changes we could do so by making us not heed the pref at all (ie use the/a builtin value) except when running tests (Cu.isInAutomation / xpc::IsInAutomation + xpcshell (bug 1598804)). I don't see any enterprise support in play here from a quick skim/search of the code.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to :Gijs (he/him) from comment #7)

Manual observer-ing seems fine to me given the parent/child dissonance problem you describe. If we want to prevent runtime changes we could do so by making us not heed the pref at all (ie use the/a builtin value) except when running tests (Cu.isInAutomation / xpc::IsInAutomation + xpcshell (bug 1598804)). I don't see any enterprise support in play here from a quick skim/search of the code.

Well, enterprise setups might set the pref, no? No need for special policy support... technically we still support user.js (and all-companyname.js in the install dir) 🙈

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #8)

(In reply to :Gijs (he/him) from comment #7)

Manual observer-ing seems fine to me given the parent/child dissonance problem you describe. If we want to prevent runtime changes we could do so by making us not heed the pref at all (ie use the/a builtin value) except when running tests (Cu.isInAutomation / xpc::IsInAutomation + xpcshell (bug 1598804)). I don't see any enterprise support in play here from a quick skim/search of the code.

Well, enterprise setups might set the pref, no? No need for special policy support... technically we still support user.js (and all-companyname.js in the install dir) 🙈

Having said that, you're probably right, I was just wondering whether there was an enterprise-related intention with these being prefs. :)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7194bd89612f
Cache the values of the CSP prefs in the ExtensionPolicyService. r=mixedpuppy

Comment 11

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7194bd89612f

Comment 12

[Alex Cornestean]

Hello,

Will this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added? Thanks!

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

I don't think so. Thank you!

Needinfo'ing Shane to give a chance to dispute my call here, since I don't have much of a relationship with this code.