Closed Bug 1366694 Opened 3 years ago Closed 2 years ago

Enable Windows level 3 content process sandbox by default on Nightly.

Categories

(Core :: Security: Process Sandboxing, enhancement)

All
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

(Blocks 1 open bug)

Details

(Whiteboard: sbwc2)

Attachments

(3 files, 1 obsolete file)

No description provided.
Whiteboard: sbwc2
Blocks: 1366697
I ran a try push with level 3 enabled and the patches from bug 1334550:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80cc6c0d859eb0108b9d304a1fc233cbf0254403

Still some issues (that I think are similar to ones we've seen on Mac) to iron out.

I also had to add in an exception for JOB_OBJECT_UILIMIT_HANDLES when in DEBUG, because the the assertion at [1] was frequently being hit in the child process as FindCOMWindow was returning null.

aklotz - do we still need this code in the child?
I don't think there's any issues with not having JOB_OBJECT_UILIMIT_HANDLES for DEBUG if that's the case.
Also, I'm slightly concerned that we might need to exclude it for opt as well, but we're just not seeing the issue on try.

[1] https://hg.mozilla.org/mozilla-central/file/23a341e9b53d/ipc/glue/WindowsMessageLoop.cpp#l151
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(aklotz)
I am kind of surprised that FindCOMWindow is failing there, but having said that, I think it is more relevant to the chrome process and to the plugin container than it is to content.
Flags: needinfo?(aklotz)
Depends on: 1378061
I've now got a leak of a11y objects in the master process in the a11y w10 x64 tests.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9598e0737b67b5d2d0fccbff404bdc44732664f8

These don't even run with e10s, so it's pretty weird, my only guess is that some interaction with the thumbnails content process is causing us to not release something in the parent.
I get loads of failures when I try and run locally, so I can't reproduce.
I've tried turning on extra leak logging but with no success at the moment, might have to hack the code to do it.

aklotz - any idea what this might be or how I'd track it down?
Flags: needinfo?(aklotz)
(In reply to Bob Owen (:bobowen) from comment #3)
> These don't even run with e10s, so it's pretty weird

a11y tests are... interesting. Pretty sure they end up spawning e10s processes even when the tests are supposedly running in non-e10s mode.

My best guess is that a document in the content process is hanging on to a reference to its parent, which is an outer doc in the parent process.

I wonder if the message releasing the reference count on that object isn't getting through to the parent...


I'm also querying the a11y people to find out if they have seen this previously.
Flags: needinfo?(aklotz)
(In reply to Bob Owen (:bobowen) from comment #3)
...
> I've tried turning on extra leak logging but with no success at the moment,
> might have to hack the code to do it.

Did this and got a stack for the unreleased allocation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c61b27e64d09eaf4a24a940e60e3a00dcc95f222
Another push with the USER_INTERACTIVE level with just the current user's SID not included in the restricting SIDs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5655aa3706156cf87943f6e42e3f112c8de1eaf

So, that appears to be what is causing the leak.
Blocks: 1379635
Blocks: 1379643
This is because in DEBUG mode we currently give full access to TEMP dir
for logging purposes and the temporary profile is created in the TEMP dir.
Attachment #8884892 - Flags: review?(jmathies)
Attachment #8884895 - Flags: review?(continuation)
Comment on attachment 8884895 [details] [diff] [review]
Part 3: Allow mochtest-a11y test to leak 248 bytes on Windows

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

I guess this is okay because it is restricted to one of the smaller test suites, and to a single OS.

::: testing/mochitest/mochitest_options.py
@@ +833,5 @@
>  
>          options.leakThresholds = {
> +            # a11y tests leak 248B when we try to run with USER_LIMITED access
> +            # token level for Windows sandbox. Bug 1379643 tracks fixing this.
> +            "default": (248 if os.name == 'nt' and options.flavor == 'a11y'

Please instead add an |if| after this statement that increments leakThresholds["default"] by 248. The way it is written now will make it  hard for anybody else to add something.
Attachment #8884895 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #12)
...
> > +            # a11y tests leak 248B when we try to run with USER_LIMITED access
> > +            # token level for Windows sandbox. Bug 1379643 tracks fixing this.
> > +            "default": (248 if os.name == 'nt' and options.flavor == 'a11y'
> 
> Please instead add an |if| after this statement that increments
> leakThresholds["default"] by 248. The way it is written now will make it 
> hard for anybody else to add something.

Thanks, changed locally.
Attachment #8884890 - Flags: review?(jmathies) → review+
Attachment #8884892 - Flags: review?(jmathies) → review+
Comment on attachment 8884897 [details] [diff] [review]
Part 4: Change Windows content process sandbox level to 3 on Nightly

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

\o/
Attachment #8884897 - Flags: review?(jmathies) → review+
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae22a66e02d
Part 1: Allow user handles in the content process job in DEBUG builds. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b71119fbf8
Part 2: Don't run sandbox file system test in DEBUG on Windows. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f7f74bee08
Part 3: Allow mochtest-a11y test to leak 248 bytes on Windows. r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7813f3652481
Part 4: Change Windows content process sandbox level to 3 on Nightly. r=jimm
backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=113288577&repo=mozilla-inbound
Flags: needinfo?(bobowencode)
No longer blocks: 1379643
Depends on: 1379643
New try push with latest patch from bug 1379643:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bfd08fc2f4b25052a122c96941391344400d62a
Flags: needinfo?(bobowencode)
Comment on attachment 8884895 [details] [diff] [review]
Part 3: Allow mochtest-a11y test to leak 248 bytes on Windows

This was more serious than I thought and is being fixed in bug 1379643, before we land.
Attachment #8884895 - Attachment is obsolete: true
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b982efa54d
Part 1: Allow user handles in the content process job in DEBUG builds. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/5195f1b94903
Part 2: Don't run sandbox file system test in DEBUG on Windows. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/60ef4d9f3023
Part 4: Change Windows content process sandbox level to 3 on Nightly. r=jimm
(In reply to Pulsebot from comment #21)
> Pushed by bobowencode@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/19b982efa54d
> Part 1: Allow user handles in the content process job in DEBUG builds. r=jimm
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5195f1b94903
> Part 2: Don't run sandbox file system test in DEBUG on Windows. r=jimm
> https://hg.mozilla.org/integration/mozilla-inbound/rev/60ef4d9f3023
> Part 4: Change Windows content process sandbox level to 3 on Nightly. r=jimm

\o/ woot!
Depends on: 1384327
Depends on: 1386502
Depends on: 1385207
Depends on: 1402325
Depends on: 1512731
You need to log in before you can comment on or make changes to this bug.