Closed Bug 1386502 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: xidorn, Assigned: bobowen)

References

Details

(Whiteboard: sb+)

Attachments

(1 file, 1 obsolete file)

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.
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?
Flags: needinfo?(jmathies)
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
Confirming this
Flags: needinfo?(jmathies)
Whiteboard: sb?
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.
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.)
See Also: → 1386826
(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.)
(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.
Can confirm on Windows 10, adding MOZ_DISABLE_CONTENT_SANDBOX=1 fixes it.
(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).
(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.
Blocks: 1366694
Flags: needinfo?(bobowencode)
Priority: -- → P1
Whiteboard: sb? → sb+
Component: Reftest → Security: Process Sandboxing
Product: Testing → Core
Looks like we'll need to white-list the source and obj dirs on Windows as well.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(bobowencode)
(I'll push to try as soon as the issues are sorted.)
Attachment #8894547 - Flags: review?(jmathies)
Attachment #8894547 - Flags: review?(jmaher)
(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 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?
Attachment #8894547 - Flags: review?(jmaher) → review+
(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.
Flags: needinfo?(agaynor)
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.
Flags: needinfo?(agaynor)
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.
Attachment #8894547 - Attachment is obsolete: true
Attachment #8894547 - Flags: review?(jmathies)
Attachment #8894623 - Flags: review?(jmathies)
Attachment #8894623 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef54a9c9f60c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.