Last Comment Bug 699633 - Inline workers broken in Firefox 8
: Inline workers broken in Firefox 8
Status: VERIFIED FIXED
[qa!]
: dev-doc-complete, qawanted, relnote, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 8 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla10
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
: 701345 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-03 16:30 PDT by Jussi Kalliokoski
Modified: 2011-12-16 08:29 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
affected
+
fixed


Attachments
Patch, v1 (4.85 KB, patch)
2011-11-03 23:09 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
christian: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑release-
Details | Diff | Splinter Review
A test for workers from blob URLs (1.23 KB, text/plain)
2011-11-04 06:02 PDT, Jussi Kalliokoski
no flags Details
A test for workers from blob URLs, v2 (1.29 KB, text/plain)
2011-11-04 06:31 PDT, Jussi Kalliokoski
no flags Details
A test for workers from dataURI (674 bytes, text/html)
2011-11-08 04:55 PST, bugzilla33
no flags Details

Description Jussi Kalliokoski 2011-11-03 16:30:18 PDT
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] 2011-11-03 16:44:15 PDT
bent, could you please look at this.
Comment 2 Olli Pettay [:smaug] 2011-11-03 16:47:55 PDT
I mean, this *looks like* a major worker problem, and workers were re-written in FF8.
Comment 3 Jussi Kalliokoski 2011-11-03 16:50:22 PDT
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!) 2011-11-03 23:09:18 PDT
Created attachment 571891 [details] [diff] [review]
Patch, v1

This fixes us.
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-03 23:10:58 PDT
(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) PTO Until July 5th 2011-11-03 23:22:06 PDT
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 2011-11-04 06:02:41 PDT
Created attachment 571941 [details]
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 2011-11-04 06:19:56 PDT
Meh, I was sloppy and that test doesn't actually work, will put up a better one a bit later.
Comment 9 Jussi Kalliokoski 2011-11-04 06:31:02 PDT
Created attachment 571948 [details]
A test for workers from blob URLs, v2

Ok, now it works.
Comment 10 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-04 09:34:16 PDT
(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!) 2011-11-04 09:34:40 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/936643625d71
Comment 12 Olli Pettay [:smaug] 2011-11-04 10:50:45 PDT
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!) 2011-11-04 10:54:25 PDT
Yes, I was just waiting to get it on m-c first.
Comment 14 Olli Pettay [:smaug] 2011-11-04 11:43:05 PDT
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 2011-11-05 02:25:32 PDT
Sure a thing, Olli, it's actually kinda a part of my job these days. :)
Comment 16 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-11-05 02:49:34 PDT
https://hg.mozilla.org/mozilla-central/rev/936643625d71
Comment 17 christian 2011-11-07 13:49:22 PST
[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 christian 2011-11-07 13:50:07 PST
Comment on attachment 571891 [details] [diff] [review]
Patch, v1

[triage comment]
Please land on mozilla-aurora today.
Comment 19 christian 2011-11-07 13:53:25 PST
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 2011-11-07 14:02:11 PST
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 2011-11-07 14:17:34 PST
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!) 2011-11-07 15:32:10 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/9addd25bb730
Comment 23 Jonas Sicking (:sicking) PTO Until July 5th 2011-11-07 18:41:11 PST
Comment on attachment 571891 [details] [diff] [review]
Patch, v1

In light of that, renominating.
Comment 24 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-08 04:12:28 PST
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 25 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-08 04:53:06 PST
*** Bug 700626 has been marked as a duplicate of this bug. ***
Comment 26 bugzilla33 2011-11-08 04:55:57 PST
Created attachment 572780 [details]
A test for workers from dataURI
Comment 27 bugzilla33 2011-11-08 04:59:01 PST
Please try attachment: A test for workers from daraURI.html

Opera 11.5 - pass
Comment 28 Masatoshi Kimura [:emk] 2011-11-08 05:08:32 PST
@bugzilla33 : That's bug 699857. This bug is about blob URLs which are created by createObjectURL.
Comment 29 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-08 05:11:14 PST
Woops, I duped his bug to the wrong one :-/
Comment 30 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-10 09:14:17 PST
*** Bug 701345 has been marked as a duplicate of this bug. ***
Comment 31 Christopher Blizzard (:blizzard) 2011-11-10 14:26:43 PST
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) PTO Until July 5th 2011-11-10 14:38:22 PST
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] 2011-11-10 20:05:32 PST
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) PTO Until July 5th 2011-11-10 23:53:30 PST
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 2011-11-11 08:16:07 PST
Jonas: I tried it and it appears to fix at least my case. :)
Comment 36 Jonas Sicking (:sicking) PTO Until July 5th 2011-11-11 10:33:47 PST
Awesome, thanks Jussi!
Comment 37 Masatoshi Kimura [:emk] 2011-11-11 13:22:27 PST
Comment on attachment 571891 [details] [diff] [review]
Patch, v1

per comment #34.
Comment 38 Alex Keybl [:akeybl] 2011-11-14 15:56:03 PST
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 40 Eric Shepherd [:sheppy] 2011-12-16 08:29:05 PST
Added a note to Firefox 9 for developers that these are working again.

Note You need to log in before you can comment on or make changes to this bug.