1249351 - Service worker installation stalls in bad state during sw-toolbox test case (or probably any importScripts of more than one script)

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[matt]

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

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Typo on my part.  It is indeed gulp.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I meant bug 1246319 comment 37.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[matt]

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

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

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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. :-)

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix, fixes the STR from comment 0

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Clean up test_importscript.html and add multiple-url importScript() case. r=bz

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

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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!

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Thank you!  Next week would be great.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with actual commit message and the like

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Test

Comment 22

[Pulsebot]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(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!

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Sylvestre Ledru [:Sylvestre]]

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 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8722173 [details] [diff] [review]
Test

Many thanks for tests, taking it too.

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Carsten Book [:Tomcat]]

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