1386502 - Reftest and crashtest doesn't run locally with e10s enabled on Windows

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Xidorn Quan [:xidorn] UTC+11]

When I execute something like
> ./mach reftest layout/reftest/css-ruby/
the reftest windows open, but they get stuck and no test runs at all.

I can see messages like:
> Chrome file doesn't exist: c:\mozilla-source\stylo\obj-firefox-stylo\_tests\reftest\specialpowers\chrome\specialpowers\content\MozillaLogger.js
> Chrome file doesn't exist: c:\mozilla-source\stylo\obj-firefox-stylo\_tests\reftest\specialpowers\chrome\specialpowers\content\specialpowersAPI.js
> Chrome file doesn't exist: c:\mozilla-source\stylo\obj-firefox-stylo\_tests\reftest\specialpowers\chrome\specialpowers\content\specialpowers.js
> Chrome file doesn't exist: c:\mozilla-source\stylo\obj-firefox-stylo\_tests\reftest\reftest\chrome\reftest\content\reftest-content.js
in the console.

Those files indeed exist, and I can run the test with --disable-e10s specified.

I suppose there might probably be some issue around sandbox.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Jim, is this a known issue? Could you suggest how should I workaround here?

I'm not sure why this doesn't break anything on CI, probably because CI doesn't run reftest and crashtest on Windows 10?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

If it's content sandboxing related, maybe some permutation of the env vars to disable can be used as a workaround?

https://wiki.mozilla.org/Security/Sandbox#Debugging_Features

Comment 3

[Jim Mathies [:jimm]]

Confirming this

Comment 4

[Daniel Holbert [:dholbert]]

I'm hitting this on Linux as well. In my case, I see this logging right after the reftest canvas window (& focus-stealing tiny-window) appear:
==========
Chrome file doesn't exist: /scratch/work/builds/mozilla-inbound/obj/_tests/reftest/reftest/chrome/reftest/content/reftest-content.js
[Child 27730] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80520015: file ../../../mozilla/dom/base/nsFrameMessageManager.cpp, line 1618
JavaScript error: file:///scratch/work/builds/mozilla-inbound/obj/dist/bin/browser/components/nsBrowserGlue.js, line 993: TypeError: win is null
[Parent 27603] WARNING: attempt to modify an immutable nsStandardURL: file /scratch/work/builds/mozilla-inbound/mozilla/netwerk/base/nsStandardURL.cpp, line 1807
[Parent 27603] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /scratch/work/builds/mozilla-inbound/mozilla/parser/html/nsHtml5StreamParser.cpp, line 977

And then it just sits there doing nothing.

Comment 5

[Daniel Holbert [:dholbert]]

I'm slowly narrowing in on a regression range on my linux box, with "hg bisect" + rebuild + ./mach reftest.  In my current "guilty" range, bug 1308400 (which tightened the sandbox on linux) is looking most-likely-suspicious.

(I assume Windows would have a somewhat different regression cause, since bug 1308400 was linux-only -- but I wouldn't be surprised if we made similar changes on both platforms recently.)

Comment 6

[Daniel Holbert [:dholbert]]

(I spun off bug 1386826 for the linux version of this issue (my comment 4 - comment 5), since it seems that this appeared on Linux due to a linux-specific change (bug 1308400), and I don't want to derail this bug here since it was filed for Windows.)

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> If it's content sandboxing related, maybe some permutation of the env vars
> to disable can be used as a workaround?
> 
> https://wiki.mozilla.org/Security/Sandbox#Debugging_Features

Yeah, adding MOZ_DISABLE_CONTENT_SANDBOX=1 makes the test run as expected, so this is indeed because of content sandbox.

(In reply to Daniel Holbert [:dholbert] (PTO August 3-7) from comment #6)
> (I spun off bug 1386826 for the linux version of this issue (my comment 4 -
> comment 5), since it seems that this appeared on Linux due to a
> linux-specific change (bug 1308400), and I don't want to derail this bug
> here since it was filed for Windows.)

The may be regressed by different bugs, but I imagine that the fix can be the same: either disable content sandbox for reftest or add the harness directory to the whitelist somehow. So it probably isn't necessary to have a separate bug for that.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Can confirm on Windows 10, adding MOZ_DISABLE_CONTENT_SANDBOX=1 fixes it.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> I imagine that the fix can be
> the same: either disable content sandbox for reftest or add the harness
> directory to the whitelist somehow. So it probably isn't necessary to have a
> separate bug for that.

Likely correct. We can dupe as-appropriate after we've got a fix somewhere.

(In reply to Mason Chang [:mchang] from comment #8)
> Can confirm on Windows 10, adding MOZ_DISABLE_CONTENT_SANDBOX=1 fixes it.

haik says in bug 1386826 comment 2 that (on linux at least) he can work around this by putting his objdir inside of his srcdir (e.g. in its default location -- just avoid setting MOZ_OBJDIR in your mozconfig).

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Daniel Holbert [:dholbert] (PTO August 3-7) from comment #9)
> (In reply to Mason Chang [:mchang] from comment #8)
> > Can confirm on Windows 10, adding MOZ_DISABLE_CONTENT_SANDBOX=1 fixes it.
> 
> haik says in bug 1386826 comment 2 that (on linux at least) he can work
> around this by putting his objdir inside of his srcdir (e.g. in its default
> location -- just avoid setting MOZ_OBJDIR in your mozconfig).

That's an interesting observation. My objdir is indeed in my srcdir, so this seems to be Linux-specific, then.

Comment 11

[Bob Owen (:bobowen)]

Looks like we'll need to white-list the source and obj dirs on Windows as well.

Comment 12

[Bob Owen (:bobowen)]

Attachment: White-list source and obj dirs for Windows content process sandbox in developer builds

(I'll push to try as soon as the issues are sorted.)

Comment 13

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #12)
> Created attachment 8894547 [details] [diff] [review]
> White-list source and obj dirs for Windows content process sandbox in
> developer builds

Just realised I need to update that comment, I'm actually whitelisting the comma delimited paths in pref security.sandbox.content.read_path_whitelist, which is what we do for Linux.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8894547 [details] [diff] [review]
White-list source and obj dirs for Windows content process sandbox in developer builds

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

test harness wise this looks good

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +343,5 @@
> +          resolvedPath.append(L"\\*");
> +        }
> +        mAllowedFilesRead.push_back(resolvedPath);
> +      }
> +    }

is there a way to not ship with this?  couldn't any addon just set this pref and then hack around the sandbox?

Comment 15

[Bob Owen (:bobowen)]

(In reply to Joel Maher ( :jmaher) (UTC-9) from comment #14)
... 
> is there a way to not ship with this?  couldn't any addon just set this pref
> and then hack around the sandbox?

Potentially yes, I'm not sure if extensions couldn't just read any file they wanted anyway.
Maybe that's not true now that (I think) web extensions will be running in their own process.

Maybe this could also just be done for IsDevelopmentBuild().
Perhaps that wouldn't work if you were running tests on a packaged build.

Alex - what do you think? (I think you hit these problems initially)

Ideally, of course we should fix the tests that rely on opening files in the content process, but I'm not sure how much work that is.

Comment 16

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1306c7050fc7c1f45608a94503506b23ae7e8544

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

Can webExtensions set prefs? I didn't think they could, if so there are worse things that they could do.

The original reason IsDevelopmentBuild() and the SRCDIR and OBJDIR variables were introduced were for handling the fact that on macOS and Linux we rely on symlinks from the built application to those directories.

I don't think it's correct to use these variables if these tests need to access these directories not-via-symlinks (or stated another way, if Windows is affected, I don't think these are the right solution :-)). Right now these values are only used with InDevelopmentBuild, and we'd very much like to avoid changing that. That said, it might make sense to use these in the short term.

Comment 18

[Bob Owen (:bobowen)]

Attachment: Whitelist paths added via pref for files opened in the content process during some tests.

Updated patch with revised comment, as I'll probably need to use checkin-needed.

Carrying r+ from jmaher in comment 14.

It looks like web extensions can't set prefs, so after Fx56 we should be OK.
Although we'd like to remove the need for these anyway to simplify things.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef54a9c9f60c
Whitelist paths added via pref for files opened in the content process during some tests. r=jimm

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef54a9c9f60c