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

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: bkelly, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

45 Branch
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45 fixed, firefox46 fixed, firefox47 fixed)

Details

(Whiteboard: dom-triaged btpp-active)

Attachments

(2 attachments, 2 obsolete attachments)

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

3 years ago
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.
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

3 years ago
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.
Whiteboard: dom-triaged btpp-active
Do you want me to investigate?  If so you need to set ni, not just CC me :)
Flags: needinfo?(bkelly)
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. :-)
Flags: needinfo?(bkelly)
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...
Attachment #8721500 - Flags: review?(bkelly)
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.
Attachment #8721500 - Flags: review?(bkelly) → review+
This stalls and times out without your fix.  It passes with the fix.
Attachment #8721570 - Flags: review?(bzbarsky)
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!
Assignee: bkelly → bzbarsky
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.  :(
Attachment #8721570 - Flags: review?(bzbarsky) → review-
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.
Attachment #8721570 - Flags: review- → review+
Summary: Service worker installation stalls in bad state during sw-toolbox test case → Service worker installation stalls in bad state during sw-toolbox test case (or probably any importScripts of more than one script)
Posted patch TestSplinter Review
Blocks: 1156847
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.
Attachment #8721500 - Flags: approval-mozilla-beta?
Attachment #8721500 - Flags: approval-mozilla-aurora?
Attachment #8721500 - Attachment is obsolete: true
Attachment #8721500 - Flags: approval-mozilla-beta?
Attachment #8721500 - Flags: approval-mozilla-aurora?
Attachment #8721570 - Attachment is obsolete: true
Attachment #8722172 - Flags: approval-mozilla-beta?
Attachment #8722172 - Flags: approval-mozilla-aurora?
Attachment #8722173 - Flags: approval-mozilla-beta?
Attachment #8722173 - Flags: approval-mozilla-aurora?
(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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a817cee8f9e6
https://hg.mozilla.org/mozilla-central/rev/c83f7bf632c1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8722172 [details] [diff] [review]
Patch with actual commit message and the like

Make sense, taking it.
Should be 45 beta 10.
Attachment #8722172 - Flags: approval-mozilla-beta?
Attachment #8722172 - Flags: approval-mozilla-beta+
Attachment #8722172 - Flags: approval-mozilla-aurora?
Attachment #8722172 - Flags: approval-mozilla-aurora+
Comment on attachment 8722173 [details] [diff] [review]
Test

Many thanks for tests, taking it too.
Attachment #8722173 - Flags: approval-mozilla-beta?
Attachment #8722173 - Flags: approval-mozilla-beta+
Attachment #8722173 - Flags: approval-mozilla-aurora?
Attachment #8722173 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.