1370438 - Permaorange in ipc xpcshell tests on OS X when Gecko 55 merges to beta on 2017-06-12

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Phil Ringnalda (:philor)]

Something that landed in the last week is going to result in massive failure in the unit_ipc xpcshell tests on OS X when we merge to beta next week, a la https://treeherder.mozilla.org/logviewer.html#?job_id=104748049&repo=try

[Tracking Requested - why for this release]: merge bustage, closed tree, delayed b1

Comment 1

[Andrew McCreight [:mccr8]]

I see a lot of odd errors, but it is hard to tell what is normal or not because regular XPCShell logs don't output anything.

Here's one that is actually causing a failure:
ReferenceError: can't access lexical declaration `redirectPath' before initialization at /builds/slave/test/build/tests/xpcshell/tests/dom/base/test/unit/test_bug553888.js:40

I'm not sure why we're hitting this, and only on OSX. There are a lot of JS language errors, but I can't imagine we'd change anything on OS X only.

Haik, is there something different about OS X sandboxing in XPCShell between Nightly and Beta? Maybe that is causing these issues? Or is there no sandboxing?

Comment 2

[Julien Cristau [:jcristau]]

this needs to be addressed before 55.0b1 so marking as blocker.

Comment 3

[Haik Aftandilian [:haik]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> I see a lot of odd errors, but it is hard to tell what is normal or not
> because regular XPCShell logs don't output anything.
> 
> Here's one that is actually causing a failure:
> ReferenceError: can't access lexical declaration `redirectPath' before
> initialization at
> /builds/slave/test/build/tests/xpcshell/tests/dom/base/test/unit/
> test_bug553888.js:40
> 
> I'm not sure why we're hitting this, and only on OSX. There are a lot of JS
> language errors, but I can't imagine we'd change anything on OS X only.
> 
> Haik, is there something different about OS X sandboxing in XPCShell between
> Nightly and Beta? Maybe that is causing these issues? Or is there no
> sandboxing?

There should be no difference right now between Nightly and Beta. And my understanding is that xpcshell tests aren't sandboxed due to Bug 1358652 "xpcshell e10s tests aren't sandboxed".

We did land Bug 1363760 "Install specialpowers as a non-temporary addon in tests" last week, but that's not Mac-specific.

Comment 4

[Andrew McCreight [:mccr8]]

I was able to reproduce this locally using two patches from philor's try run, on OSX. I went through and compared a successful log to a failing log, because there is a lot of error spew even in a passing run.

The first divergent badness seems to be a failure in httpd.js:

20:11:35     INFO -  CHILD-TEST-STARTED
20:11:35     INFO -  PID 6497 | [Child 6498] WARNING: failed to bind socket: file /home/worker/workspace/build/src/netwerk/base/nsServerSocket.cpp, line 360
20:11:35     INFO -  PID 6497 | !!! could not start server on port -1: [Exception... "Component returned failure code: 0x804b000d (NS_ERROR_CONNECTION_REFUSED) [nsIServerSocket.init]"  nsresult: "0x804b000d (NS_ERROR_CONNECTION_REFUSED)"  location: "JS frame :: resource://testing-common/httpd.js :: _start :: line 550"  data: no]

We're trying to start an httpd.js in a content process, and getting refused.

Then I read over bug 1358652 comment 0, which Haik pointed out to me in comment 3, and thought, gee, that sounds a lot like what I'm seeing. Anyways, I poked around in the code a bit, and it turns out that bug 1358223 made it so that the content process sandbox level is hardcoded to 1 in non-Nightly builds.

This also applies to content processes started in XPCShell, and thus as jld predicted, these tests break. I locally confirmed that commenting out the chunk of code inside the "#if !defined(NIGHTLY_BUILD) && (defined(XP_WIN) || defined(XP_MACOSX))" block in GetEffectiveContentSandboxLevel() makes test_bug553888_wrap.js pass again.

Can you take a look at this, Alex? I'm not sure what the right way to fix this for XPCShell tests is...

Comment 5

[Andrew McCreight [:mccr8]]

It might also be worth looking at why these tests didn't break on Windows, too. I don't know if that's expected.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Ouch. Sorry about this!

I don't know why this wouldn't have affected Windows, I'd have expected it to break Windows and macOS, but not Linux.

It looks like properly fixing up xpcshell tests to pass under the sandbox is a pretty large scale project. The options I see are either:

a) Set the |MOZ_DISABLE_CONTENT_SANDBOX| environment variable when running xpcshell tests. That should give you the same effective behavior as setting the pref to 0, and is probably the most correct way to do this. I don't know much about the xpcshell runner, so hopefully someone can chime in.
b) Backout/partially backout the patch on beta. (Partial backout could be acheived by commenting out line 19-21 of SandboxSettings.cpp)

Comment 7

[Andrew McCreight [:mccr8]]

Maybe buildCoreEnvironment in testing/xpcshell/runxpcshelltests.py is the right place to set an environment variable? Those look like pretty generic settings.

Comment 8

[Phil Ringnalda (:philor)]

Windows is broken, https://treeherder.mozilla.org/logviewer.html#?job_id=104763022&repo=try, but very lightly and only in crashreporter and underneath another separate bustage, so I hadn't yet spotted it as being the same thing.

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Oh, on further thinking: the thing that's failing is binding a socket, I'm not sure the Windows sandbox prohibits that. That'd explain why the bustage is macOS only.

buildCoreEnvironment sounds like a plausible location for adding the environment variable.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1370438 - The MOZ_DISABLE_CONTENT_SANDBOX environment variable now works on macOS and is used in the xpcshell tests;

This environment variable works on both Windows and Linux for force-disabling
the content sandbox, and now does so on macOS as well.

The xpcshell tests force disable the sandbox because they do things like bind()
sockets, which is not compatible with the content sandbox. This is needed now
because bug 1358223 was force upgrading the sandbox from level 0 (disabled) to
level 1 on beta channel, which caused breakage.

Review commit: https://reviewboard.mozilla.org/r/146856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146856/

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Patch is small, so hopefully we can have this landed with plenty of time before the merge to beta.

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

Comment 12

[Haik Aftandilian [:haik]]

Comment on attachment 8875404 [details]
Bug 1370438 - The MOZ_DISABLE_CONTENT_SANDBOX environment variable now works on macOS and is used in the xpcshell tests;

https://reviewboard.mozilla.org/r/146856/#review150922

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5a453664f8f
The MOZ_DISABLE_CONTENT_SANDBOX environment variable now works on macOS and is used in the xpcshell tests; r=haik

Comment 14

[Carsten Book [:Tomcat]]

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