1784517 - Work around bug 1667271 with some skip-if's in sandboxing test manifests

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, task, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

From bug 1667271 comment #89 it seems that some tools will interpret any file named moz.build regardless of whether it would normally be part of a given build, including to find test manifests, and it would be nontrivial to fix that.

So, as a workaround to prevent broken CI test runs on sandboxing changes (see bug 1667271 comment #48), we can add something like skip-if = asan || tsan || ccov (that may not be the exact right condition) to the [DEFAULT] section of the test manifests under security/sandbox, to match the conditions where the configure scripts will exclude that subtree from the build.

This isn't great — for one thing, there will be the potential for the skip-ifs to diverge from the build configuration — but it should be simple and save the sheriffs from dealing with these false positives.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1784517 - Explicitly skip sandboxing tests on unsandboxed build types.

There are tools which consume moz.build files by reading every one in
the tree, rather than traversing DIRS for a specific build type (see
bug 1667271 comment #89 for background); as a result, we can end up with
CI test jobs that try to run sandboxing tests on build types like Linux
ASan where security/sandbox isn't built, and fail.

This patch applies a suggested workaround: add an otherwise redundant
skip-if declaration to the test manifests to skip them on the platforms
where they're not part of the build.

Note that sandboxing is disabled in the presence of Linux ASan or TSan
by logic in toolkit/moz.configure, but for code coverage builds it's
done via the CI mozconfig files adding --disable-sandbox.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Following feedback on Phabricator I tried making this patch simpler by adding a sandbox binding here, in mozinfo.py, so that I can skip the tests based on the actual MOZ_SANDBOX config setting instead of reproducing the logic for when it's usually enabled. It seems to work locally with mach test, but on Try it doesn't appear to run any of the security/sandbox tests when it should (from searching the full logs for security/sandbox; with unmodified m-c, I do see that string in the logs). But also, if I change the patch to add a not_sandbox value instead, then it does work — i.e., the binding I added is always being read as false, suggesting that it's not being picked up by some part of the test scheduling process. Any ideas about how to proceed?

Comment 3

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

this is really odd- the tests should run no matter what, so I am confused why they didn't run in the first instance when you set sandbox=true in mozinfo. I see you had use try chooser in both try pushes.

I admit to be baffled by this, the original try push commit looks like the right thing. I guess the real mystery is why is this failing to schedule security/sandbox/test/browser* tests on your try push. Given that this works locally, the patch overall looks right, there has to be some small detail that I am overlooking.

Comment 4

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

there is some logic in taskcluster to chunk the test suites smarter- part of this is skipping manifests where no tests run based on mozinfo data- the key is mozinfo data is sort of "hardcoded" and if there is a new key, it isn't known what the value is, so what was happening is sandbox was assumed to be false, so !sandbox was evaluating to true in ALL situations.

adding some basic logic for sandbox helps here:
https://hg.mozilla.org/try/rev/b805b979273f15ab9fa8385b010a76e0731169f2#l7.12

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Under the circumstances, I think I'll stick with the original version of my patch; it's a little hacky, but the alternative is that I'd have to do basically the same thing in the fake mozinfo for test chunking instead of directly in the manifests.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/65f4d203964e
Explicitly skip sandboxing tests on unsandboxed build types. r=jmaher

Comment 7

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/65f4d203964e