Closed Bug 1067301 Opened 5 years ago Closed 5 years ago

Add mach / mochitest argument to enable the Windows content sandbox

Categories

(Core :: Security, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(1 file, 1 obsolete file)

Add mach / mochitest argument to enable the Windows content sandbox similar to the --e10s argument to enable e10s.
Blocks: 1067305
This adds a new option of --content-sandbox (off,warn,on) to run mochitests via mach with the Windows content sandbox enabled.

It assumes --e10s as you can't have the content sandbox without it.
Attachment #8489382 - Flags: review?(jmaher)
Comment on attachment 8489382 [details] [diff] [review]
Add mach / mochitest option to run tests with Windows content sandbox.

Review of attachment 8489382 [details] [diff] [review]:
-----------------------------------------------------------------

just one question, otherwise this looks good.

::: testing/mochitest/mach_commands.py
@@ +489,5 @@
>      this_chunk = CommandArgument('--e10s', action='store_true',
>          help='Run tests with electrolysis preferences and test filtering enabled.')
>      func = this_chunk(func)
>  
> +    this_chunk = CommandArgument('--content-sandbox', default='off', choices=['off', 'warn', 'on'],

this works consistently across windows,linux,osx,android,b2g?
Attachment #8489382 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #2)
> > +    this_chunk = CommandArgument('--content-sandbox', default='off', choices=['off', 'warn', 'on'],
> 
> this works consistently across windows,linux,osx,android,b2g?

Assuming I'm following the patch correctly and it just sets a pref: it'll be a no-op on the Linux platforms.  Specifically: content process sandboxing is always enabled if supported on B2G (which includes TBPL emulator runs since February (bug 932098)), and is currently config'ed off on Linux desktop and Android (and would probably break lots of stuff if enabled).  Also, non-B2G Android devices typically don't have support for it.
(In reply to Jed Davis [:jld] from comment #3)
> (In reply to Joel Maher (:jmaher) from comment #2)
> > > +    this_chunk = CommandArgument('--content-sandbox', default='off', choices=['off', 'warn', 'on'],
> > 
> > this works consistently across windows,linux,osx,android,b2g?
> 
> Assuming I'm following the patch correctly and it just sets a pref: it'll be
> a no-op on the Linux platforms.  Specifically: content process sandboxing is
> always enabled if supported on B2G (which includes TBPL emulator runs since
> February (bug 932098)), and is currently config'ed off on Linux desktop and
> Android (and would probably break lots of stuff if enabled).  Also, non-B2G
> Android devices typically don't have support for it.

Yeah, I suspect the pref would get set, but only Windows uses it, so it would have no effect if someone used it on other platforms.
r=jmaher - from comment 2

(In reply to Joel Maher (:jmaher) from comment #2)

> > +    this_chunk = CommandArgument('--content-sandbox', default='off', choices=['off', 'warn', 'on'],
> 
> this works consistently across windows,linux,osx,android,b2g?

I'm glad you raised this as when I did some testing on this, I noticed that because I was defaulting the parameter in run_desktop_test to None this meant that if the new argument wasn't specified it was always turning on e10s.

This patch just defaults content_sandbox to 'off' instead.
The pref does always get set, but shouldn't affect other platforms.

Try push on most platforms with this change:
https://tbpl.mozilla.org/?tree=Try&rev=3f77ac7a5d49
Attachment #8489382 - Attachment is obsolete: true
Attachment #8489924 - Flags: review+
Try push showing that tests are unaffected without the new option specifiedn:
https://tbpl.mozilla.org/?tree=Try&rev=3f77ac7a5d49

Thanks.
Keywords: checkin-needed
I'm on PTO for a week, so I'll re-request when I get back.
Keywords: checkin-needed
Just checked this still applies cleanly to m-c.

Try push showing that tests are unaffected without the new option specified:
https://tbpl.mozilla.org/?tree=Try&rev=3f77ac7a5d49

Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0fce2a1d8ab6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.