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

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Comment 1

[Bob Owen (:bobowen)]

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

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 3

[Bob Owen (:bobowen)]

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?

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(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.

Comment 5

[Bob Owen (:bobowen)]

(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

Comment 6

[Bob Owen (:bobowen)]

try push with just USER_LIMITED change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f389234183224d4aa160bcd72bdba2a8ee97dbb7

try push with just JOB_RESTRICTED change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45f61b2d6e29d92e3f06cbfa52652b1505e652f2

Comment 7

[Bob Owen (:bobowen)]

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.

Comment 8

[Bob Owen (:bobowen)]

Attachment: Part 1: Allow user handles in the content process job in DEBUG builds

Comment 9

[Bob Owen (:bobowen)]

Attachment: Part 2: Don't run sandbox file system test in DEBUG on Windows

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.

Comment 10

[Bob Owen (:bobowen)]

Attachment: Part 3: Allow mochtest-a11y test to leak 248 bytes on Windows

Comment 11

[Bob Owen (:bobowen)]

Attachment: Part 4: Change Windows content process sandbox level to 3 on Nightly

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=370da14f1857a251ec18a359e1365c08a85da8f8

Comment 14

[Bob Owen (:bobowen)]

(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.

Comment 15

[Jim Mathies [:jimm]]

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/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Carsten Book [:Tomcat]]

backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=113288577&repo=mozilla-inbound

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bed082fc54c6
Backed out changeset 7813f3652481 
https://hg.mozilla.org/integration/mozilla-inbound/rev/28eaa0476b17
Backed out changeset 37f7f74bee08 
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0dca99a081
Backed out changeset 88b71119fbf8 
https://hg.mozilla.org/integration/mozilla-inbound/rev/2afdc1569a3d
Backed out changeset 2ae22a66e02d for memory leaks

Comment 19

[Bob Owen (:bobowen)]

New try push with latest patch from bug 1379643:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bfd08fc2f4b25052a122c96941391344400d62a

Comment 20

[Bob Owen (:bobowen)]

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.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Jim Mathies [:jimm]]

(In reply to Pulsebot from comment #21)
> Pushed by [e-mail]:
> 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!

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/19b982efa54d
https://hg.mozilla.org/mozilla-central/rev/5195f1b94903
https://hg.mozilla.org/mozilla-central/rev/60ef4d9f3023