Lazily create common sandbox broker policy
Categories
(Core :: Security: Process Sandboxing, enhancement, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox80 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
The SandboxBrokerPolicyFactory was created as a place to cache the parts of the content sandbox filesystem policy that are shared by all content processes (i.e., don't depend on the pid or if it's a file:/// process, etc.). This was especially important on B2G, when the policy was a simple set of paths (no prefix rules, etc.) and populating it included recursive filesystem traversal on a low-end phone, but there's still work that probably makes sense to cache, especially with Fission increasing the number of content processes.
However, it's initialized at a point before user-changed prefs are available, so pref-derived settings were being recomputed for every process (as seen in bug 1621231). And now bug 1640345 would make a lot more settings pref-derived that currently aren't.
I have patches to make the common policy construction happen lazily the first time a content process policy is requested, and move as much as possible back into it.
|
||
Updated•4 years ago
|
|
Assignee | |
Comment 1•4 years ago
|
||
Not strictly necessary, but I noticed this while I was making changes:
AddDynamicPathList can be a simple static function instead of a private
static method, and doesn't need to be in the header.
|
|
Assignee | |
Comment 2•4 years ago
|
||
When the SandboxBrokerPolicyFactory is constructed, prefs aren't
available, which constrains the cached subset of the content process
policy to entries that don't depend on prefs. Delaying the computation
until a content process is started removes that restriction.
|
|
Assignee | |
Comment 3•4 years ago
|
||
Now that filesystem broker policy entries that depend on prefs can be
cached in the "common" policy object, let's do this wherever possible.
Should also fix bug 1621231.
Pushed by jedavis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ef0321787c8f Part 0: Make AddDynamicPathList a static non-member function. r=gcp https://hg.mozilla.org/integration/autoland/rev/54904fe41df8 Part 1: Construct content sandbox "common" policy lazily. r=gcp https://hg.mozilla.org/integration/autoland/rev/4da77f0a0687 Part 2: Cache as much of the content sandbox file policy as possible. r=gcp
|
||
Comment 5•4 years ago
|
||
Backed out 4 changesets (Bug 1644917, Bug 1640345) for causing failures in browser_preferences_usage.js CLOSED TREE
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307967950&repo=autoland&lineNumber=7884
Backout: https://hg.mozilla.org/integration/autoland/rev/adc328596e28636b03fabe701ec6a4d07054e5af
|
|
Assignee | |
Comment 6•4 years ago
|
||
I thought I'd run Try on this patch stack but it looks like I never did; sorry about that.
The error message is a little confusing — the test was expecting that certain sandbox prefs would be accessed too much (49-55 times after 50 cross-site navigations in Fission mode, vs. a default limit of 40), but now they're completely absent from the pref stats, because the number of accesses during the test is now zero.
Pushed by jedavis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/517da8786030 Part 0: Make AddDynamicPathList a static non-member function. r=gcp https://hg.mozilla.org/integration/autoland/rev/4ae86938c750 Part 1: Construct content sandbox "common" policy lazily. r=gcp,Gijs https://hg.mozilla.org/integration/autoland/rev/c19de94931de Part 2: Cache as much of the content sandbox file policy as possible. r=gcp,Gijs
|
||
Comment 8•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/517da8786030
https://hg.mozilla.org/mozilla-central/rev/4ae86938c750
https://hg.mozilla.org/mozilla-central/rev/c19de94931de
Description
•