Closed Bug 896582 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jsmith, Assigned: martijn.martijn)

References

Details

Attachments

(1 file, 7 obsolete files)

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
Blocks: 896587
Attached patch all.diff (obsolete) — Splinter Review
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.
Attached patch wippatch.diff (obsolete) — Splinter Review
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
Attachment #779942 - Attachment is obsolete: true
Attached patch wippatch2.diff (obsolete) — Splinter Review
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.
Attachment #779953 - Attachment is obsolete: true
Attached patch wippatch2.diff (obsolete) — Splinter Review
Sorry, some stupid mistake, which made the tryserver run fail, new one: https://tbpl.mozilla.org/?tree=Try&rev=70c80dfde9f8
Attachment #780101 - Attachment is obsolete: true
Attached patch wippatch3.diff (obsolete) — Splinter Review
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
Attachment #780246 - Attachment is obsolete: true
Attached patch 896582_media.diff (obsolete) — Splinter Review
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.
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.
Attached patch 896582_media2.diff (obsolete) — Splinter Review
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
Attachment #780333 - Attachment is obsolete: true
Attachment #780578 - Attachment is obsolete: true
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.
Attachment #780901 - Flags: review?(jgriffin)
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.
(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 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?
Attachment #780901 - Flags: review?(jgriffin) → review+
(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.
(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.
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.
Attachment #780901 - Attachment is obsolete: true
Did you mean to flag in checkin-needed btw per comment 15?
Yeah, apparently, I forgot.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/740efbc3485a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Hmm, what's the story with these Web Audio tests which time out here?
Sorry, you meant this broke tbpl on mozilla-central? Feel free to back out!
(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.
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.
(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?
It seems like the same bug, but I don't really know.
(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.
Assignee: nobody → martijn.martijn
(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.
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... :/
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?
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.
You need to log in before you can comment on or make changes to this bug.