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

Conversation

Projects
None yet
5 participants
@nornagon
Contributor

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

@nornagon nornagon added the wip label Nov 28, 2018

@nornagon nornagon requested a review from electron/reviewers as a code owner Nov 28, 2018

@nornagon nornagon force-pushed the mixed-sandbox-linux branch from 5bdce3e to b0b66bf Nov 28, 2018

@ppontes

This comment has been minimized.

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.

@nornagon nornagon referenced this pull request Nov 29, 2018

Open

feat: enable mixed-sandbox mode by default #15894

4 of 4 tasks complete

@nornagon nornagon force-pushed the mixed-sandbox-linux branch from a137eea to b34e727 Nov 29, 2018

@miniak

This comment has been minimized.

Contributor

miniak commented Dec 3, 2018

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

@nornagon

This comment has been minimized.

Contributor

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 force-pushed the mixed-sandbox-linux branch from b34e727 to 398e9fc Dec 3, 2018

nornagon added some commits Dec 4, 2018

@nornagon nornagon removed the wip label Dec 5, 2018

@nornagon

This comment has been minimized.

Contributor

nornagon commented Dec 5, 2018

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

@deepak1556

LGTM 👍

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Dec 6, 2018

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

@nornagon

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

@zcbenz

zcbenz approved these changes Dec 6, 2018

@nornagon nornagon merged commit 2845267 into master Dec 6, 2018

21 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP Legacy commit status override — see details
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

release-clerk bot commented Dec 6, 2018

Release Notes Persisted

mixed-sandbox mode now works on Linux

@nornagon

This comment has been minimized.

Contributor

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 Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment