Closed Bug 1067305 Opened 10 years ago Closed 10 years ago

Disable failing media mochitests using the contentSandbox property in test manifests.

Categories

(Core :: Security, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(1 file, 1 obsolete file)

Use the property added in bug 1067301, to disable the media mochitests that work with e10s.
Blocks: 1074147
This disables the media tests that currently fail with the content sandbox.

Hopefully the win32 plain mochitests will be running on holly soon, so disabling these help to see any regressions elsewhere.

Here's a try run with this change and the sandbox enabled:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cac92d4e92dc

The failures in M-1 and M-5 are separate regressions, they were passing at one point, so I'll look at these separately.

You can also see that we fail in Win XP completely ... that's my next job. :)

As I was writing this I realised I should do a push without the --content-sandbox option to show that these tests still run normally:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=025e1e4e158f
Attachment #8498175 - Flags: review?(jmaher)
Comment on attachment 8498175 [details] [diff] [review]
Conditionally disable mochitests that won't run with the content sandbox at low integrity.

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

thanks for having a bug to track these.
Attachment #8498175 - Flags: review?(jmaher) → review+
Try push showing the tests skipped when content sandbox is enabled via --content-sandbox:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cac92d4e92dc

Other failures here are being worked on, but this is only with the content sandbox and so doesn't affect normal test runs.

Try push showing the tests run when --content-sandbox is not used:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=025e1e4e158f

Tests under content/media/test and content/media/webaudio/test run in M-1
Tests under dom/media/tests/mochitest run in M-3

Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/645d5e930e23
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Guys, is it possible that this patch has dis-abled all of the WebRTC mochitest?!
Locally on my Mac not a single test gets executed any more. And I see an error message at the start of mochitest that it ignores all my tests because of the missing sandbox!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So here is what I run:

$ ./mach mochitest dom/media/tests/mochitest/test_peerConnection_basicAudio.html 
make: Entering directory `/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin13.4.0'
From _tests: Kept 20575 existing; Added/updated 0; Removed 0 files and 0 directories.
make: Leaving directory `/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin13.4.0'
TEST-SKIPPED | dom/media/tests/mochitest/test_peerConnection_basicAudio.html | skip-if: (contentSandbox != 'off' ) || (toolkit == 'gonk' )

The conclusion that contentSandbox is not off is wrong. I'm on Mac. I did not provide the content-sandbox argument to mochitest.
If I change the line in dom/media/tests/mochitest/mochitest.ini you added:
  skip-if = contentSandbox != 'off' # contentSandbox(Bug 1042735)
to this
  skip-if = contentSandbox == 'on' # contentSandbox(Bug 1042735)
test start to execute as expected. If I comment out the whole line it also works.

Luckily tests still seem to get executed on try. Not sure how the build servers invoke this any different so they still execute the tests properly.

BTW if I understand bug 1042735 correct the problem only exists on Windows (as also the sandbox only exists on Windows so far), right?
So something like this would have been nice:
  skip-if = os == "win" && contentSandbox != 'off' # contentSandbox(Bug 1042735)
You disabled all the content/media mochitests, on Windows at least, nothing is working. This needs to be backed out.

In future, please request review from the module owners before messing with their tests.
Chris, Nils - really sorry that I disabled these tests in some circumstances when the sandbox wasn't enabled.
Clearly this was not my intention.

(In reply to Nils Ohlmeier [:drno] from comment #7)
> So here is what I run:
> 
> $ ./mach mochitest
> dom/media/tests/mochitest/test_peerConnection_basicAudio.html 
> make: Entering directory
> `/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin13.4.0'
> From _tests: Kept 20575 existing; Added/updated 0; Removed 0 files and 0
> directories.
> make: Leaving directory
> `/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin13.4.0'
> TEST-SKIPPED | dom/media/tests/mochitest/test_peerConnection_basicAudio.html
> | skip-if: (contentSandbox != 'off' ) || (toolkit == 'gonk' )
> 
> The conclusion that contentSandbox is not off is wrong. I'm on Mac. I did
> not provide the content-sandbox argument to mochitest.
> If I change the line in dom/media/tests/mochitest/mochitest.ini you added:
>   skip-if = contentSandbox != 'off' # contentSandbox(Bug 1042735)
> to this
>   skip-if = contentSandbox == 'on' # contentSandbox(Bug 1042735)
> test start to execute as expected. If I comment out the whole line it also
> works.

It would have to be something like: contentSandbox in ['warn', 'on']
and those options may change, which was why I used: != 'off'
This should be the default, but clearly that is not getting set for locally run tests, in at least some circumstances.

> Luckily tests still seem to get executed on try. Not sure how the build
> servers invoke this any different so they still execute the tests properly.

I had checked m-i when this landed (and try before) to make sure these tests were still running.
I thought I'd checked for locally run tests as well, but I obviously didn't test thoroughly enough.
 

(In reply to Chris Pearce (:cpearce) from comment #8)
> You disabled all the content/media mochitests, on Windows at least, nothing
> is working. This needs to be backed out.
> 
> In future, please request review from the module owners before messing with
> their tests.

I wasn't aware that this was the policy for changes to tests.
However, I should have at least got feedback from a media peer, so that they were aware of the changes and let people know in #media as well.
So, apologies again for that, but no malicious disabling of tests, just an honest mistake. :)
(In reply to Bob Owen (:bobowen) from comment #10)
> Chris, Nils - really sorry that I disabled these tests in some circumstances
> when the sandbox wasn't enabled.
> Clearly this was not my intention.

Yes, I don't believe it was intentional. Apology accepted. :)

> (In reply to Chris Pearce (:cpearce) from comment #8)
> > You disabled all the content/media mochitests, on Windows at least, nothing
> > is working. This needs to be backed out.
> > 
> > In future, please request review from the module owners before messing with
> > their tests.
> 
> I wasn't aware that this was the policy for changes to tests.

We have a review policy somewhere?

> However, I should have at least got feedback from a media peer, so that they
> were aware of the changes and let people know in #media as well.

Basically yes. I lost several hours on Friday and yesterday tracking down why my code spontaneously starting failing, which delayed sending a build off to a partner. If I'd known up front we'd had a change here, I would have found the issue sooner.

> So, apologies again for that, but no malicious disabling of tests, just an
> honest mistake. :)

Ok, no hard feelings. :)
(In reply to Chris Pearce (:cpearce) from comment #11)

> > However, I should have at least got feedback from a media peer, so that they
> > were aware of the changes and let people know in #media as well.
> 
> Basically yes. I lost several hours on Friday and yesterday tracking down
> why my code spontaneously starting failing, which delayed sending a build
> off to a partner. If I'd known up front we'd had a change here, I would have
> found the issue sooner.

Ah, I thought that the only effect of this error was to cause tests to be skipped when running locally through mach.
Is there some other effect, that caused your code to start failing, that I need to take into account when I re-land?

> > So, apologies again for that, but no malicious disabling of tests, just an
> > honest mistake. :)
> 
> Ok, no hard feelings. :)

Excellent, maybe I should be the one buying a beer for you and Ryan in Portland. :)
(In reply to Bob Owen (:bobowen) from comment #12)
> Ah, I thought that the only effect of this error was to cause tests to be
> skipped when running locally through mach.

You thought correctly, that was the effect.

> Is there some other effect, that caused your code to start failing, that I
> need to take into account when I re-land?

Nope, we just need the tests to keep working on Windows and other platforms.

> Excellent, maybe I should be the one buying a beer for you and Ryan in
> Portland. :)

mmmmmmm beeerrrr
The actual problem here was that mach applied the filters using the standard mozinfo at build time, before calling the mochitest scripts.
It is the mochitest scripts that add the contentSandbox mozinfo (and e10s as well) and then filter again.
So, any negative tests against these meant that the tests would already be disabled at build time.

Trying to get our extra context into the standard mozinfo would be really painful, so I'd just decided that we should turn off the build-time filtering for mochitests, when I noticed that bug 1074507 has just landed and got rid of the build time filtering altogether.
Here's a new patch with the |os == 'win'| suggestion from Nils added, just to be on the safe side. I had to rebase the patch anyway.
The expression parser for skip-if can't handle |in| just |==| and |!=|, so I've left it as |contentSandbox != 'off'|.

I've tested this locally with mach on Windows and Linux.

Here's a try push with the new patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=293a72905dc0

Although the test servers were never a problem, I'll check that the tests are running on all platforms before I land.

Chris - are you happy that I re-land this?
(Assuming the jmaher is happy with the |os =='win'|)
Attachment #8498175 - Attachment is obsolete: true
Attachment #8501900 - Flags: review?(jmaher)
Flags: needinfo?(cpearce)
Comment on attachment 8501900 [details] [diff] [review]
Conditionally disable media mochitests that won't run with the content sandbox at low integrity.

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

these manifest conditions look correct.
Attachment #8501900 - Flags: review?(jmaher) → review+
(In reply to Bob Owen (:bobowen) from comment #15)
> Created attachment 8501900 [details] [diff] [review]
> Conditionally disable media mochitests that won't run with the content
> sandbox at low integrity.
> 
> Here's a new patch with the |os == 'win'| suggestion from Nils added, just
> to be on the safe side. I had to rebase the patch anyway.
> The expression parser for skip-if can't handle |in| just |==| and |!=|, so
> I've left it as |contentSandbox != 'off'|.

Ok this will fix the issue I had running mach mochitest locally on my Mac.
 
> I've tested this locally with mach on Windows and Linux.
> 
> Here's a try push with the new patch:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=293a72905dc0
> 
> Although the test servers were never a problem, I'll check that the tests
> are running on all platforms before I land.
> 
> Chris - are you happy that I re-land this?
> (Assuming the jmaher is happy with the |os =='win'|)

But if I understood Chris problem, he tried to run mochitest locally on his Windows system and ran into the same issue as me, that mochitest would not execute any tests any more.
I don't quite get how your new patch is suppose to fix that issue?!
(In reply to Bob Owen (:bobowen) from comment #15)
> Created attachment 8501900 [details] [diff] [review]
> ...
> Chris - are you happy that I re-land this?
> (Assuming the jmaher is happy with the |os =='win'|)

I tested this patch, and my tests still work on Windows in normal non-e10s builds. So no opposition from me to land this.

Indeed, it seems that the landing of bug 1074507 actually makes your earlier patch (without the os=='win' check) work too (on Windows) and it seems to me that your earlier patch would be fine too (though I didn't test on any platforms other than Windows).

So it seems that contentSandbox wasn't working correctly without bug 1074507 landed?
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #18)

> Indeed, it seems that the landing of bug 1074507 actually makes your earlier
> patch (without the os=='win' check) work too (on Windows) and it seems to me
> that your earlier patch would be fine too (though I didn't test on any
> platforms other than Windows).
> 
> So it seems that contentSandbox wasn't working correctly without bug 1074507
> landed?

Right, on the test servers it was fine, but locally because of the filtering done by mach in the build, contentSandbox was not defined at this point and so |!= 'off'| meant they were always disabled.
The secondary filtering in the mochitest scripts could only disable tests further not re-enable them.

I'd just come to the conclusion that I should remove that build filtering for mochitests as it was not needed any more.
When I checked to see when it had been added, I found that all the build filtering had just been removed yesterday. :-)

While |contentSandbox != 'off'| will now work on its own and is currently only used by Windows, Nils is right that this media issue is specific to the Windows content sandbox at the moment so I think the extra condition makes sense.
https://hg.mozilla.org/mozilla-central/rev/99444136ae76
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.