Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support mixed-sandbox mode on linux #15870

Merged
merged 5 commits into from Dec 6, 2018
Merged

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Nov 28, 2018

Description of Change

This adds a patch to Chromium to allow mixing --no-zygote renderers with regular zygote-launched renderers.

This currently only works when launching the app with --enable-mixed-sandbox on the command line—when using the app.enableMixedSandbox API, the zygote process is launched before the JS initialization code is run, and thus fails to pick up the mixed-sandbox flag and launches with --no-sandbox, resulting in a weird mix of zygote-launched and non-zygote-launched renderers all of which are without sandbox. My preferred fix for that problem is to remove the --enable-mixed-sandbox flag and have it be the default (overridden by --no-sandbox or --enable-sandbox).

I'm not exactly sure how this will interact with site-per-process and navigation. In particular, will this behave correctly if you load a.com in a sandboxed renderer and then navigate to b.com with site-per-process enabled? (Further, what's the status of site-per-process? cc @ppontes)

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Release Notes

Notes: mixed-sandbox mode works on Linux

@ppontes
Copy link
Member

ppontes commented Nov 29, 2018

@nornagon, site-per-process is also WIP: 15821. We're addressing one last issue before we can attempt to merge it.

@miniak
Copy link
Contributor

miniak commented Dec 3, 2018

@nornagon would it make sense to keep two zygotes? one sandboxed and one non-sandboxed?

@nornagon
Copy link
Member Author

nornagon commented Dec 3, 2018

possibly, but it would be a rather more involved change and it's not clear to me that the zygote is providing substantial value other than optimizing sandbox setup (see e.g. rsesek's comment here). It may indeed be a worthwhile improvement, but without data to back it up I don't think it's worth going through the effort of supporting a 2nd zygote.

If we were to go down the 2-zygote path, we'd also want to consider lazily initializing the zygotes, so that if an app is using all-sandboxed or all-unsandboxed renderers, they don't have to pay the memory cost of an additional process.

@nornagon nornagon removed the wip ⚒ label Dec 5, 2018
@nornagon
Copy link
Member Author

nornagon commented Dec 5, 2018

ping @zcbenz @deepak1556, this is ready for review :)

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@deepak1556
Copy link
Member

Can we have a tracking issue for #15870 (comment) ? Could lead to a potential enhancement.

@nornagon
Copy link
Member Author

nornagon commented Dec 6, 2018

I don't think it makes sense to have a tracking issue at this time, given we haven't even evaluated whether it would be an improvement or not.

@deepak1556
Copy link
Member

deepak1556 commented Dec 6, 2018

given we haven't even evaluated whether it would be an improvement or not.

The tracking issue in my mind was for this specifically (evaluate the perf benefits of having a zygote), don't want it to get lost in PR comments :)

@nornagon
Copy link
Member Author

nornagon commented Dec 6, 2018

don't want it to get lost in PR comments

want it to get lost in the issue tracker instead? :D

@nornagon nornagon merged commit 2845267 into master Dec 6, 2018
@release-clerk
Copy link

release-clerk bot commented Dec 6, 2018

Release Notes Persisted

mixed-sandbox mode now works on Linux

@nornagon
Copy link
Member Author

nornagon commented Dec 6, 2018

Merging even though I still have some concerns about the way this might interact with site-per-process, we can revert / fix forward if we discover issues.

@nornagon nornagon deleted the mixed-sandbox-linux branch December 6, 2018 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants