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

Support multiprocess constellation on Windows #13609

Closed
wants to merge 1 commit into from

Conversation

@vvuk
Copy link
Contributor

vvuk commented Oct 6, 2016

Allow -M on windows to use multiprocess mode. Sandboxing is still disallowed, as gaol is not implemented on Windows. This depends on servo/ipc-channel#108 to provide windows support in ipc-channel. At the moment this causes a panic in ipc-channel when run with multiprocess (works fine single process), but I wanted to put this up so that others can test.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Oct 6, 2016

Heads up! This PR modifies the following files:

@@ -530,13 +529,40 @@ impl UnprivilegedPipelineContent {
Ok(child_process)
}

// gaol is not supported on Windows yet, so this is just multiprocess
// without sandboxing

This comment has been minimized.

@jdm

jdm Oct 13, 2016

Member

My preference would be to add a cfg block to the existing non-Windows code rather than duplicating the non-sandbox pieces here.

This comment has been minimized.

@vvuk

vvuk Oct 20, 2016

Author Contributor

The reason I didn't do that is that we don't have the gaol crate available on Windows at all. cfg!(...) evaluates to a boolean, so the types would still need to be available for both paths through the function. Is there a way to do this that I'm not seeing? The only other way I could think of was to create separate spawn_sandboxed() and spawn_unsandboxed() methods that spawn_multiprocess calls, and then have a windows-only spawn_sandboxed that panics.

With my latest changes at servo/ipc-channel#108, multiprocess Servo works on Windows.

@bors-servo
Copy link
Contributor

bors-servo commented Dec 14, 2016

The latest upstream changes (presumably #14536) made this pull request unmergeable. Please resolve the merge conflicts.

@nox
Copy link
Member

nox commented Mar 3, 2017

@jdm Are you ok with repeating these few lines of code for now? Should we land this?

@jdm
Copy link
Member

jdm commented Mar 3, 2017

We need servo/ipc-channel#108 to merge before this can. I'm ok with the code duplication.

@nox nox closed this Mar 24, 2017
@nox nox reopened this Mar 24, 2017
@jdm
Copy link
Member

jdm commented Nov 15, 2017

No need to keep this open indefinitely.

@jdm jdm closed this Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.