699633 - Inline workers broken in Firefox 8

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jussi Kalliokoski]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111026191032

Steps to reproduce:

Go to http://labs.avd.io/sink.js/tests/callback.html (turn your volume down, if not using Firefox 8)

The site uses sink.js that is a library that provides a consistent API for audio output across Firefox and Chrome, and because of the timers clamping in Firefox 5+, I had to devise a mechanism that allowed me time accurate workers in background tabs as well, but didn't include the cruft of the end-developer having to include another JS file that the worker is loaded from. So, there is Sink.doInterval(callback, timeout) that attempts to create an inline worker using blob urls. It's try-catching everything, so if there's an error, it just goes with the regular timers. This worked in Firefox 6-7, however it no longer seems to work on Firefox 8. You can find out what doInterval does from https://github.com/jussi-kalliokoski/sink.js/blob/master/sink.js#L517 . This is related to an earlier bug related to the timers clamping change https://bugzilla.mozilla.org/show_bug.cgi?id=665000 . And just like that time, there's even no way to feature detect this.

I cannot stress enough the importance of this feature. If any application that uses the Audio Data API to play back some audio wants to do that also when in a background tab (any music streaming application, you don't keep them in the open tab, do you?), this is needed to work without making the codebase a horrible cruft. Just look at http://jsmad.org for example.

</rant> :)


Actual results:

[00:57:15.160] Failed to load script: moz-filedata:207126df-bbc2-46e1-8754-fa3ae9c0014c (nsresult = 0x80004002)


Expected results:

You should hear some terrible generated noise.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

bent, could you please look at this.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I mean, this *looks like* a major worker problem, and workers were re-written in FF8.

Comment 3

[Jussi Kalliokoski]

Yep, to me it looks like you can't create workers from blob urls anymore.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This fixes us.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jussi Kalliokoski from comment #3)
> Yep, to me it looks like you can't create workers from blob urls anymore.

Who knew this worked! Yikes.

The script loading part was almost totally unchanged from the last implementation, but in the old implementation I think we were a little more lax when calculating stuff for the location object.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 571891 [details] [diff] [review]
Patch, v1

We need tests too. You can create a Blob using the MozBlobBuilder interface.

Comment 7

[Jussi Kalliokoski]

Attachment: A test for workers from blob URLs

I don't have the time to download the sources on my laptop right now and validate that the test works, but I think it should do the trick.

Comment 8

[Jussi Kalliokoski]

Meh, I was sloppy and that test doesn't actually work, will put up a better one a bit later.

Comment 9

[Jussi Kalliokoski]

Attachment: A test for workers from blob URLs, v2

Ok, now it works.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jussi Kalliokoski from comment #9)
> Created attachment 571948 [details]
> A test for workers from blob URLs, v2

Thanks! I'll incorporate that test into the patch.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/936643625d71

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 571891 [details] [diff] [review]
Patch, v1

Should we get this to branches.
It would be rather unfortunate to break all sorts of
audio web apps.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yes, I was just waiting to get it on m-c first.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Jussi, I wonder if we could get more tests for your audio libraries to prevent this kinds of
regressions. If you think you could write such tests, please file a new bug and attach the patch there.

Comment 15

[Jussi Kalliokoski]

Sure a thing, Olli, it's actually kinda a part of my job these days. :)

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/936643625d71

Comment 17

[LegNeato]

[triage comment]

We'll definitely take this in aurora / Fx9. Fx 8 is shipping tomorrow. What's the fallout of shipping with this? Do you have any idea as to the extent of sites using workers with binary blobs?

Comment 18

[LegNeato]

Comment on attachment 571891 [details] [diff] [review]
Patch, v1

[triage comment]
Please land on mozilla-aurora today.

Comment 19

[LegNeato]

Comment on attachment 571891 [details] [diff] [review]
Patch, v1

[triage comment]

We will not be respinning Firefox 8 for this based on the information we have. We have not heard about any high-volume web apps this is breaking, QA's tests are passing, and we haven't got feedback from our testing community saying we broke this. If you have examples that we should consider, please renominate. Denied for beta/Firefox 8.

Comment 20

[Jussi Kalliokoski]

Nothing high volume I guess, but basically any audio app that relies on sink.js or audiolib.js (jsmad, the proof of js performance, though)... However, turns out html5rocks has a tutorial on the subject, so I have no idea if it's actually more widespread: http://www.html5rocks.com/en/tutorials/workers/basics/#toc-inlineworkers-bloburis .

Comment 21

[Jussi Kalliokoski]

Oh yeah, also, anything using Audiolet ( https://github.com/oampo/Audiolet ) will be broken because it's using sink.js as well. It's breaking basicly most of the complex audio demos out there.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9addd25bb730

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 571891 [details] [diff] [review]
Patch, v1

In light of that, renominating.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We're shipping in hours, so you shouldn't be nominating for approval, you should be emailing release-drivers convincing them to pull the train's emergency brake.

Comment 26

[bugzilla33]

Attachment: A test for workers from dataURI

Comment 27

[bugzilla33]

Please try attachment: A test for workers from daraURI.html

Opera 11.5 - pass

Comment 28

[Masatoshi Kimura [:emk]]

@bugzilla33 : That's bug 699857. This bug is about blob URLs which are created by createObjectURL.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Woops, I duped his bug to the wrong one :-/

Comment 31

[Christopher Blizzard (:blizzard)]

We want to take this as a ride-along for an 8.0.1 update.  We need to get some testing to make sure that the fix that we're taking works with the audio sites in question.  Can we please test with nightlies and once we have an 8.0.1 test build?

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Jussi: if you download a build from http://nightly.mozilla.org, does everything work as it should there?

If you can verify that it does work, then we might be able to fix this for Firefox 8.0.1

Comment 33

[Alex Keybl [:akeybl]]

Comment on attachment 571891 [details] [diff] [review]
Patch, v1

[Triage Comment]
Approved for beta. Please land ASAP. We're considering this for an 8.0.1 and want to make sure that it lands elsewhere first, to prevent surprises.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Alex, this was landed on aurora before the merge on tuesday, so it's already on beta.

Do you want me to land it on mozilla-release?

Comment 35

[Jussi Kalliokoski]

Jonas: I tried it and it appears to fix at least my case. :)

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Awesome, thanks Jussi!

Comment 37

[Masatoshi Kimura [:emk]]

Comment on attachment 571891 [details] [diff] [review]
Patch, v1

per comment #34.

Comment 38

[Alex Keybl [:akeybl]]

Comment on attachment 571891 [details] [diff] [review]
Patch, v1

[Triage Comment]
Denying for release as we are no longer considering shipping 8.0.1 to all Firefox users - it will be a targeted release for those who are, or could potentially be, affected by bug 700835 or bug 691271.

Comment 39

[Ioana (away)]

Verfied as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0 (20111116091359)
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111121 Firefox/10.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111122 Firefox/11.0a1

The test case from comment 0 was used.

The automated test case (dom/workers/test/test_blobWorkers.html	) is also passing on al OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7486436&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=7485704&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=7485705&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=7484699&full=1&branch=mozilla-beta

https://tbpl.mozilla.org/php/getParsedLog.php?id=7516724&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7515744&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7516005&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7514311&full=1&branch=mozilla-aurora

https://tbpl.mozilla.org/php/getParsedLog.php?id=7528338&full=1&branch=mozilla-inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=7528321&full=1&branch=mozilla-inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=7529122&full=1&branch=mozilla-inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=7528050&full=1&branch=mozilla-inbound

Comment 40

[Eric Shepherd [:sheppy]]

Added a note to Firefox 9 for developers that these are working again.