896582 - Implement a workaround to bug 838726 to get a bunch of content/media mochitests running on B2G

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jason Smith [:jsmith]]

bug 838726 will currently will lead to causing many of the content/media mochitests to fail with something like the following:

00:03:50 INFO - 3 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_invalid_reject.html | We expected 'tests/content/media/test/320x240.ogv' to exist, but it doesn't!

Martijn mentions there's a workaround we can do here that Android considered doing here in manifest.js:

http://mxr.mozilla.org/mozilla-central/source/content/media/test/manifest.js#213

As see at that line of code, we should consider adding a B2G here like the following:

SpecialPowers.Services.appinfo.name == "B2G"

Let's see if we can implement a workaround to get around bug 838726 to be able to get many of these content/media mochitests running.

Reference Try Run:

https://tbpl.mozilla.org/?tree=Try&rev=20a337b39982

Comment 1

[Martijn Wargers (dead)]

Attachment: all.diff

Wip patch, that at least completes all the media tests, althought there are still 25 failures to be worked out.
I have some major issues on my vm with Ubuntu and some files getting random read-only.
I'll post a follow-up patch to be used on tryserver.

Comment 2

[Martijn Wargers (dead)]

Attachment: wippatch.diff

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=b9415d926437

Some notes:
- Iirc, all of the testfiles I added in the exclude list, were due to timeouts (no idea why they are happening)
- test files that are opened by window.open don't have a SpecialPowers object
- There is all kinds of netscape.enablePrivilege code lying around in this directory that has to be changed at some point

Comment 3

[Martijn Wargers (dead)]

Attachment: wippatch2.diff

wippatch2 pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d59d4ffa3e2e

Getting some different failures locally, which is strange, but I just added them to the exclude list for now.

Comment 4

[Martijn Wargers (dead)]

Attachment: wippatch2.diff

Sorry, some stupid mistake, which made the tryserver run fail, new one: https://tbpl.mozilla.org/?tree=Try&rev=70c80dfde9f8

Comment 5

[Martijn Wargers (dead)]

Attachment: wippatch3.diff

Fixed some obvious mistake in test_decoder_disable and added some test files in b2g that were reliably failing: https://tbpl.mozilla.org/?tree=Try&rev=eb128b70ea84

Comment 6

[Martijn Wargers (dead)]

Attachment: 896582_media.diff

Final patch (hopefully), I'll do a try server run on this patch, shortly (first I'll make sure this works at least reasonably well, locally).

I have just added content/media/test/test_decoder_disable.html to the exclude list for now, it should be easy to fix, but I'll do that later.
Also added content/media/test/test_can_play_type.html to the exclude list.

Comment 7

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3d6d5130387c

There is the issue where b2g mochitests fail to even start up when the b2g.json file gets too big, see bug 817638, comment 37.
On my Macbook Pro, I noticed that with the extra entries in the exclude list, time to start up for loading the mochitest app, then loading the first test takes appr. 85s.
Without the entries in the exlude list, time to start up for loading the mochitest app, then loading the first test takes appr. 75s.

Comment 8

[Martijn Wargers (dead)]

Attachment: 896582_media2.diff

Added these to the exclude list:
    "content/media/test/test_can_play_type_ogg.html":"",
    "content/media/test/test_can_play_type_no_dash.html":"",
Because in the last try run, they caused failures in chunk 2.

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=1e90fb5259ea

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 780901 [details] [diff] [review]
896582_media2.diff

Ok, latest try is all green.
Fyi, chunk 3 of mochitest b2g takes the longest, 87 minutes, much longer than the other ones.

Comment 10

[Jason Smith [:jsmith]]

Comment on attachment 780901 [details] [diff] [review]
896582_media2.diff

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

::: content/media/test/manifest.js
@@ +680,3 @@
>                                   .classes["@mozilla.org/preferences-service;1"]
>                                   .getService(SpecialPowers.Ci.nsIPrefService);
>    var branch = prefService.getBranch("media.");

Nit - If we're referencing SpecialPowers directly, do we still need this variable defined?

::: testing/mochitest/b2g.json
@@ +4,5 @@
>      "content": "",
>      "docshell": "",
>      "dom": "",
> +    "layout": "",
> +    "content/media/":""

You've already specified content above, so I don't think you need to specify this here as well.

Comment 11

[Martijn Wargers (dead)]

(In reply to Jason Smith [:jsmith] from comment #10)
> Comment on attachment 780901 [details] [diff] [review]
> 896582_media2.diff
> 
> Review of attachment 780901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/test/manifest.js
> @@ +680,3 @@
> >                                   .classes["@mozilla.org/preferences-service;1"]
> >                                   .getService(SpecialPowers.Ci.nsIPrefService);
> >    var branch = prefService.getBranch("media.");
> 
> Nit - If we're referencing SpecialPowers directly, do we still need this
> variable defined?

Yes, you're right. But I just quickly made that test file passing on b2g for now. This test file needs to be further improved by using pushPrefEnv.

> ::: testing/mochitest/b2g.json

> You've already specified content above, so I don't think you need to specify
> this here as well.

Yes, you're right, I'll remove it, thanks.

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 780901 [details] [diff] [review]
896582_media2.diff

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

Ditto Jason's comments, but looks good.

::: content/media/test/file_access_controls.html
@@ -149,5 @@
>  }
>  
>  function done() {
> -  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> -  mediaTestCleanup();

Are we certain removing this is completely harmless?

Comment 13

[Martijn Wargers (dead)]

(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> >  function done() {
> > -  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > -  mediaTestCleanup();
> 
> Are we certain removing this is completely harmless?

Yes! On the contrary even, if I don't remove it, this would cause failures in the b2g platorm, because popup windows there currently don't have the SpecialPowers object (mediaTestCleanup() is doing a SpecialPowers.forceGC()).
But when the popup is closed opener.done() is called and that function contains a mediaTestCleanup() anyway.

If you don't mind, I'd rather not change manifest.js not further at this point. I have to change this file to use pushPrefEnv anyhow (and I would remove that stuff then), but this a rather extensive change that could effect a whole bunch of files, see 868439 comment 18.

Comment 14

[Jonathan Griffin (:jgriffin)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #13)
> (In reply to Jonathan Griffin (:jgriffin) from comment #12)
> > >  function done() {
> > > -  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > > -  mediaTestCleanup();
> > 
> > Are we certain removing this is completely harmless?
> 
> Yes! On the contrary even, if I don't remove it, this would cause failures
> in the b2g platorm, because popup windows there currently don't have the
> SpecialPowers object (mediaTestCleanup() is doing a SpecialPowers.forceGC()).
> But when the popup is closed opener.done() is called and that function
> contains a mediaTestCleanup() anyway.
> 
> If you don't mind, I'd rather not change manifest.js not further at this
> point. I have to change this file to use pushPrefEnv anyhow (and I would
> remove that stuff then), but this a rather extensive change that could
> effect a whole bunch of files, see 868439 comment 18.

Ok, thanks for the explanation, and sounds fair.

Comment 15

[Martijn Wargers (dead)]

Attachment: 896582_media3.diff

For check-in.

Note for whoever this checks in: there is a chance this would cause b2g mochitest chunks to die on start-up, see bug 817638, comment 37. This is not something that is caught by tryserver. Please backout if that happens, thanks.

Comment 16

[Jason Smith [:jsmith]]

Did you mean to flag in checkin-needed btw per comment 15?

Comment 17

[Martijn Wargers (dead)]

Yeah, apparently, I forgot.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/740efbc3485a

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/740efbc3485a

Comment 20

[(no longer active)]

Hmm, what's the story with these Web Audio tests which time out here?

Comment 21

[Martijn Wargers (dead)]

Sorry, you meant this broke tbpl on mozilla-central? Feel free to back out!

Comment 22

[Jason Smith [:jsmith]]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #21)
> Sorry, you meant this broke tbpl on mozilla-central? Feel free to back out!

I think what Ehsan is talking about here is asking a question about why some of the webaudio tests are turned off in the b2g.json file. Specifically:

1. content/media/webaudio/test/test_audioBufferSourceNodeNeutered.html
2. content/media/webaudio/test/test_audioBufferSourceNodeOffset.html

[1] appears to be timing out. The second one doesn't have an indication of why.

Comment 23

[Martijn Wargers (dead)]

Oh, ok. I'll be working on adding the error messages back (or at least some message of why they are failing). I removed them, because there was some concern it caused failures at start up of the b2g mochitest run, due to the large b2g.json file.

Comment 24

[Jason Smith [:jsmith]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> Hmm, what's the story with these Web Audio tests which time out here?

I filed bug 898693 for the timeout for audioBufferSourceNodeNeutered.

For audioBufferSourceNodeOffset, this appears to be a test failure:

03:24:56     INFO -  212 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_audioBufferSourceNodeOffset.html | Correct number of samples received (expected: 24000, actual: 58880).

I see bug 895720 that might be similar to this. Is this the same issue as bug 895720? Or should I file a separate bug for this?

Comment 25

[Martijn Wargers (dead)]

It seems like the same bug, but I don't really know.

Comment 26

[Martijn Wargers (dead)]

(In reply to Jason Smith [:jsmith] from comment #24)
> I filed bug 898693 for the timeout for audioBufferSourceNodeNeutered.

This can probably safely be removed from the b2g.json file, see bug 898693, comment 1.

Comment 27

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #6)
> I have just added content/media/test/test_decoder_disable.html to the
> exclude list for now, it should be easy to fix, but I'll do that later.

This is now part of bug 898696.

Comment 28

[(no longer active)]

I mostly meant to point out that when disabling tests like this, it's really helpful to involve the module owner in the discussion.  I found out about this by accident... :/

Comment 29

[Martijn Wargers (dead)]

I didn't disable any tests here, I only enabled tests.

I'm working on adding comments back in the b2g.json exclude list, filing bugs for any new issue that I think I'm seeing and filing them in the component of what it is testing. Does that help?

Comment 30

[Martijn Wargers (dead)]

This introduced a bunch of intermittent failures, btw. Those tests were readded to the exclude list. See bug 901732 and bug 901716.

I filed bug 902677 for the fact that a lot of content/media tests are timing out in b2g, currently.