Matt Gaunt reports that if you: 1) git clone https://github.com/GoogleChrome/sw-toolbox.git 2) git checkout 312839e77b9d61d9dde226cd3085fa336dfde8de 4) npm install; npm install grunt; grunt test:manual 5) Load localhost:8888 in firefox The service worker script will stall. Upon restarting the service worker registration cannot be unregistered. Running this locally I see: Error: Script terminated by timeout at: [12]</<@http://ubuntu:8888/sw-toolbox.js:40:1324 EventListener.handleEvent*[12]<@http://ubuntu:8888/sw-toolbox.js:40:914 s@http://ubuntu:8888/sw-toolbox.js:17:613 e@http://ubuntu:8888/sw-toolbox.js:17:792 @http://ubuntu:8888/sw-toolbox.js:17:378 @http://ubuntu:8888/sw-toolbox.js:17:317 @http://ubuntu:8888/sw-toolbox.js:17:2 @http://ubuntu:8888/test/browser-tests/router-get/serviceworkers/origin-matching.js:21:1 And a broken registration in about:serviceworkers: Origin: http://ubuntu:8888 Scope: http://ubuntu:8888/test/iframe/145582008400926 Script Spec: Current Worker URL: Active Cache Name: Waiting Cache Name: Push Endpoint: null Matt noticed that if he changes this line: importScripts('/sw-toolbox.js', '/test/data/skip-and-claim.js'); To: importScripts('/sw-toolbox.js'); importScripts('/test/data/skip-and-claim.js'); Then everything works. I suspect that skipWaiting() is executing before the rest of the install handler and doing bad things.

It might need to be gulp rather than grunt unless there is something clever there.

Typo on my part. It is indeed gulp.

The broken registration in about:serviceworkers is a recent regression described in bug 1246319 comment 31. Lets see if this is still a problem after the fix for that merges to nightly.

I meant bug 1246319 comment 37.

Note you need to use this git rev to reproduce the problem: ae6ed61cd97173e6693681ceae1fe3a6807e8dbd The rev in comment 0 has the workaround in place. With this I see the tests stall once they hit "Test router.get method" cases.

Just in case it helps - the router.get methods are the tests that register service workers with the multiple importScript arguments. Refreshing the page after hitting the failing tests should illustrate the previous tests (there were passing) now fail.

I see start the ScriptLoader.cpp code with 2 URLs, but we never get to its completed state. So this seems to be a bug in the ScriptLoader somewhere.

Do you want me to investigate? If so you need to set ni, not just CC me :)

I was going to NI you if I didn't figure it out today. But it seems I might have gotten Boris look at it. :-)

Ben and I poked at this a bit. The problem is that when doing serviceworker cache stuff we're storing mReader on the script loader runnable, but that runnable handles _all_ the script loads. So we end up having multiple loadinfos all waiting on the same stream, but only one gets notified. We need to store a separate stream for each loadinfo that's doing cache stuff.

Not sure about the right name for this member, or how to create tests for this...

And with that patch, reloading the page reruns the tests correctly too.

Comment on attachment 8721500 [details] [diff] [review] Fix, fixes the STR from comment 0 Review of attachment 8721500 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I'll see if I can write a test quick tonight.

This stalls and times out without your fix. It passes with the fix.

Oh, I also cleaned up the test a little bit in the patch. That comment was wrong. A non-existent script should throw. Since I'm going on PTO, do you mind getting this landed and uplifted? Ideally we would like it in beta, but I know its late in the cycle. Thanks!

Yeah, I can get this in. But not until Monday.

Thank you! Next week would be great.

Comment on attachment 8721570 [details] [diff] [review] P2 Clean up test_importscript.html and add multiple-url importScript() case. r=bz This is missing lorum_script.js. I'm not sure what that needs to contain to reliably reproduce this bug. :(

Comment on attachment 8721570 [details] [diff] [review] P2 Clean up test_importscript.html and add multiple-url importScript() case. r=bz I guess I'll try making lorum_script.js just contain a short lorem ipsum string. That seems to trigger a failure on this test... r=me with that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a817cee8f9e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/c83f7bf632c1

Comment on attachment 8721500 [details] [diff] [review] Fix, fixes the STR from comment 0 Approval Request Comment [Feature/regressing bug #]: Bug 1156847. [User impact if declined]: Some totally legitimate uses of service workers will end up completely wedging the service worker, requiring a browser restart to recover. Oh, and browser shutdown won't really complete either. [Describe test coverage new/current, TreeHerder]: Patch has test, and we have various other serviceworker tests. [Risks and why]: I think the risk is very low (never thought I'd say this about worker code). This will only affect importScripts() calls with more than one argument, will only affect them in the serviceworker case, and will change them from hanging to not hanging. [String/UUID change made/needed]: None. Note that I realize the regressing bug is claimed to be fixed in 40. But serviceworkers in general were off by default until 44 (see bug 1226686), and this bug seems bad enough that I think we should uplift it to 45... As people use serviceworkers more now that we've enabled them, they're more and more likely to hit this.

(In reply to Boris Zbarsky [:bz] from comment #19) > Comment on attachment 8721570 [details] [diff] [review] > P2 Clean up test_importscript.html and add multiple-url importScript() case. > r=bz > > I guess I'll try making lorum_script.js just contain a short lorem ipsum > string. That seems to trigger a failure on this test... > > r=me with that. Yes, it was a file with 10k bytes of lorum ipsum string. Sorry I forgot the hg add!

https://hg.mozilla.org/mozilla-central/rev/a817cee8f9e6 https://hg.mozilla.org/mozilla-central/rev/c83f7bf632c1

Comment on attachment 8722172 [details] [diff] [review] Patch with actual commit message and the like Make sense, taking it. Should be 45 beta 10.

Comment on attachment 8722173 [details] [diff] [review] Test Many thanks for tests, taking it too.

https://hg.mozilla.org/releases/mozilla-aurora/rev/05e30972c2c5 https://hg.mozilla.org/releases/mozilla-aurora/rev/ef2ac0d2bc7e

https://hg.mozilla.org/releases/mozilla-beta/rev/61b0482fa504 https://hg.mozilla.org/releases/mozilla-beta/rev/aff9cfe1c67a