1644917 - Lazily create common sandbox broker policy

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1644917 - Part 0: Make AddDynamicPathList a static non-member function.

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.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1644917 - Part 1: Construct content sandbox "common" policy lazily.

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.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1644917 - Part 2: Cache as much of the content sandbox file policy as possible.

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
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

[Noemi Erli[:noemi_erli]]

Backed out 4 changesets (Bug 1644917, Bug 1640345) for causing failures in browser_preferences_usage.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=I4JjFG4AR8mDDVcT2gfYNg.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=0f8b6494d3eb9dad4c7f455f5ee89ad2a2d77e5b

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

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/517da8786030
https://hg.mozilla.org/mozilla-central/rev/4ae86938c750
https://hg.mozilla.org/mozilla-central/rev/c19de94931de