Open Bug 1476101 Opened Last year Updated 6 days ago

Optimize Windows sandbox policy construction

Categories

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

x86_64
Windows
enhancement

Tracking

()

People

(Reporter: jld, Assigned: emalysz, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p2:responsiveness][fxperf:p2])

Attachments

(2 files, 1 obsolete file)

I profiled content process launch on Windows, and found that we spend about half the total ContentParent::LaunchSubprocess time constructing the sandbox policy: https://perfht.ml/2zM88PS

Specifically, we spend a lot of time adding the filesystem rules, and more specifically in sandbox::IsReparsePoint and sandbox::ConvertToLongPath.

But the policy is mostly (or entirely?) the same for every process, so ideally we could just create the common policy once and copy it per-process like we did on B2G, or maybe cache that work some other way.


Data-driven warning: We don't have telemetry data on what portion of launch time this is for the median user.  The CONTENT_PROCESS_LAUNCH_TIME_MS metric has some bugs (it's collecting times from two different points in the process), but I'm seeing a median around 8ms on the system where I profiled that vs. 34ms on Beta, so that profile isn't exactly representative.  Bug 1474991 would help with this.
Priority: -- → P2
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1:f67]

Hey bobowen, I hear you work on the Windows sandbox! :) What are the chances this can be prioritized sometime this quarter?

Flags: needinfo?(bobowencode)
Whiteboard: [qf:p1:f67] → [qf:p2:responsiveness][fxperf]

After reviewing the sandbox policy code, I don't see an obvious reason why we can't share them (when the policy settings are the same of course). In fact the policy code itself appears to have been created with that in mind.
Some of the other chromium sandbox code that uses it pretty much assumes a single process per policy, but it doesn't look like it will require big changes.

I'll try and take a stab at a patch soon.
Leaving the NI to nag me.

(In reply to Bob Owen (:bobowen) from comment #2)

I'll try and take a stab at a patch soon.
Leaving the NI to nag me.

Any progress to report here, bobowen? Can we assign this one to you?

Whiteboard: [qf:p2:responsiveness][fxperf] → [qf:p2:responsiveness][fxperf:p2]

I'm not actively working on this, but I will try to when I get a chance.

Things aren't quite so simple as I first thought in comment 2, as there is some information in the policy that changes per process, but it doesn't look too bad.

Hello, I'm an Outreachy applicant and I will start working on this bug.

See Also: → 1541246
Assignee: nobody → emalysz

Proposed plan:

  1. Change policy by splitting out dynamic parts and passing those by another means to SpawnTarget
  2. Check use of policy object and avoid any sharing problems.
  3. Let SandboxBroker hold 1 common process

To be clear, this ought to have no impact on startup time, yes? As we'll still have to do the work for the first content process, we'll just be avoiding it for subsequent launches? I imagine there's no way we have in mind for reducing this time for the first content process launch of a session, correct?

Do we know how often UI changes are blocked on content process creation, vs when we are able to complete process creation in the background?

Attachment #9091594 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.