1226443 - delay service worker updates triggered by events

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Brendan Dahl [:bdahl]]

STR

1) Load https://adriftwith.me/sandbox/service_worker_reload/go.html
2) Reload the page
3) Reload again

Expected:
On the second reload there should be a message "new ServiceWorker installed"

Actual:
The service worker is never updated

In chrome I see the expected behavior above. I'm not exactly sure what the spec expects use to do, but not updating the service worker on reload makes development more painful, since you either have to trigger a navigation event or do a registration.update() on every page to get the new service worker.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I'm looking at this.

Note that the console.log() for the install event does appear in the webconsole.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Brendan, can you change your code to look for registration.installing || registration.waiting?  Per the spec we go to waiting immediately if there is not already a waiting worker.  This effectively races with the dispatch of the onupdatefound.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Nevermind, your script would be throwing if that was the case.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Brendan, it seems your code assumes the .ready promise will always resolve before the onupdatefound event.  Its late at night here, but I don't think the spec guarantees that.

Can you try placing your onupdatefound event on the registration returned from service worker .register()?

Or can you point me at what synchronizes these two events in the spec such that activation will always occur prior to starting the installation of the new service worker script?

I'll look more tomorrow as well.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

To clarify, though, the service worker is updating on refresh in current nightly.  Your just missing the onupdatefound event.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Some other information for what I saw:

* Refreshing your online site I did see the "new ServiceWorker installed" text maybe 20% of the time.
* When I copied the site locally and used a node.js script to generate the sw.js timestamps I saw the "new ServiceWorker installed" text 100% of the time.

I think this behavior also suggests a race between the ready promise and the onupdatefound.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Actually, this is spec'd.

The spec says:

1) Handle the fetch for the navigation
2) If the active worker is still in activating, wait until activated
3) fire fetch event
4) do update

The ready promise fires when the worker becomes activated in (2).  The onupdatefound is triggered in (4).

In gecko, however, we don't currently wait for the active worker to go to activated.

I agree this is a problem for developers.  We should implement bug 1226441.

Comment 8

[:Harald Kirschner :digitarald]

Thanks bdahl and bkelly for digging in!

I actually had the same behaviour yesterday and worked around it by listening to the 'statechange' event and waiting for the old worker to be redundant: https://github.com/mozilla/platatus/pull/162/files#diff-99a85c3932c0670278158f38d818e0efR21 . Glad to hear that this is spec'd!

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Brendan, Harald, its possible this fix won't make it into 44.  Do you think its bad enough for developers to block the release?  Again, the updates do occur, but we fire the ready promise late.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

After sleeping on it I think we need to do something for this before shipping.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

So, I have a patch in bug 1226441 that makes fetch event wait for the activate event to complete.  It even has a new wpt test demonstrating that it works.

It doesn't, however, make this demo site work more consistently.  I'm investigating.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Ok, I think this is a spec bug:

  https://github.com/slightlyoff/ServiceWorker/issues/788

We should probably wait to trigger the update until after the target document has loaded.  Patch forthcoming.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Another good reason to delay the update is to avoid regression document load times with extra main thread runnables.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Delay navigation update until after DOM content is loaded. r=ehsan

Ehsan, welcome back!  Have a review..

I tested this locally and confirmed it make the demo act consistent on each refresh.  In addition, we match chrome's behavior here (although they delay even more).

I opened a spec bug to try to get this standardized:

  https://github.com/slightlyoff/ServiceWorker/issues/788

The one question I am unsure of is if DOMContentLoaded will fire if the page errors out and goes to about:neterror.  If it does, then we can probably remove the failure case that triggers an update in ServiceWorkerPrivate fetch event dispatch path.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06c3d1de3556

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #14)
> The one question I am unsure of is if DOMContentLoaded will fire if the page
> errors out and goes to about:neterror.  If it does, then we can probably
> remove the failure case that triggers an update in ServiceWorkerPrivate
> fetch event dispatch path.

I just confirmed that DOMContentLoaded does not fire in this case.  So we do need the ServiceWorkerPrivate code.

In other words, the patch does not need to change again.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Investigating try failures.  The test_skip_waiting failure is a bit mysterious at first glance...

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Delay navigation update until after DOM content is loaded. r=ehsa

Slight modification not to use SWM if the nsIServiceWorkerManager is not available.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Don't reset service worker registration script URL during soft update. r=ehsan

I believe the test_skip_waiting.html failures were due to a race in the spec.  I describe it here:

  https://github.com/slightlyoff/ServiceWorker/issues/789

Basically, the navigation update was racing with a .register(differentScriptURL) on the page.  With the right timing the navigation update would wipe the new script URL specified in the .register().

I believe we should just skip the update in this case as its clear a registration change is already in process.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cd008dd3cbd

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Disable some unregister-then-register-new-script.https.html test cases. r=ehsan

So I think we were passing the tests in unregister-then-register-new-script.https.html by accident due to our update timing and the dubious reset of the registration script URL by the update.

We really have a bug in our .register() method as described in bug 1227007.  We are running some code async which should really be sync.  This causes us not to clear the uninstalling flag before the frame.remove() in these tests.

This is very corner casey, though, so I propose we mark these expected fail for now.  I think its more important to run updates consistently when the document can see the updatefound event.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bd79e6befc5

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Unfortunately still some intermittent orange in the try.  Timeouts in skip-waiting.https.html and a crash in test_sanitize_domain.html.  Unfortunately I've run out of time to work this before my PTO.  If anyone wants to track these down, that would be great.  Otherwise I will look when I get back on Nov 30.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8690565 [details] [diff] [review]
P2 Don't reset service worker registration script URL during soft update. r=ehsan

Since this isn't going to land yet, I think we should probably just do the full change described in bug 1227015 instead of this P2 patch.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

It would be nice to get this in 44, but I don't think it should block.  It is a company issue with chrome, though.

Comment 25

[(no longer active)]

Comment on attachment 8690564 [details] [diff] [review]
P1 Delay navigation update until after DOM content is loaded. r=ehsa

Review of attachment 8690564 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +3058,5 @@
> +
> +  // We perform these success path navigation update steps when the
> +  // document tells us its more or less done loading.  This avoids
> +  // slowing down page load and also lets pages consistently get
> +  // updatefound events when they fire.

Can we delay this even further?  Like a second after the document has been loaded, similar to what Chrome does?

(I'm r+ing this as it fixes what the bug was about, but I really think we should delay more.  Do you mind doing that in a follow-up, please?)

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #25)
> (I'm r+ing this as it fixes what the bug was about, but I really think we
> should delay more.  Do you mind doing that in a follow-up, please?)

Thanks.  I might do it here if it lets me sidestep some of the races in the spec similar to what chrome does now.  I'll look at it this week.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

I'm disabling some update wpt tests in bug 1230164.  They navigation and update race which makes the test flakey.  We should re-enable in this bug.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: work in progress

A work-in-progress to change how the delay is done.

I think it would be better to mark a registration as needing up and then scheduler a timer.  Any update requests cancel and reschedule the timer further out.  When the timer finally fires it does just a single update.

Scheduling the timer will occur at document load time and at functional event time.  We will track whether the update should check 24 hour time or not.  For example, if a navigation and functional event both schedule updates in the same timer window, then no 24 hour check is done because the navigation takes precedence.

This will supersede the P1 patch.  P2 is no longer needed after bug 1227015.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a timer based mechanism for firing service worker updates. r=ehsan

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make service worker fetch and functional events used scheduled timer updates. r=ehsan

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Re-enable service worker update wpt tests. r=ehsan

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dde1124795e9

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a timer based mechanism for firing service worker updates. r=ehsan

Updated to explicitly cancel any update timers when a registration is cleared.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08ec2084e8a4

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a timer based mechanism for firing service worker updates. r=ehsan

Updated to cleanup the timers on browser shutdown.

The try looked rather green except for the leak, so flagging for review.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make service worker fetch and functional events used scheduled timer updates. r=ehsan

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Re-enable service worker update wpt tests. r=ehsan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2488d33ef9e0

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a timer based mechanism for firing service worker updates. r=ehsan

One more version that checks mShuttingDown when a timer fires.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57dd44c3e200

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a timer based mechanism for firing service worker updates. r=ehsan

Sorry for all the flag spam.  I realized we should also avoid adding new timers if mShuttingDown is true.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a timer based mechanism for firing service worker updates. r=ehsan

Sorry again.  I realized I wasn't checking for the 24 hour threshold when the timer fired for a purely function event update.

I'm not going to change this any more until after review.  Sorry again for the churn.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dff9255d3eac

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8697167 [details] [diff] [review]
P1 Add a timer based mechanism for firing service worker updates. r=ehsan

Hmm, there is still a shutdown leak somehow.  Sorry, I have to investigate that.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Cleanup ServiceWorkerScriptCache objects when initialization fails. r=ehsan

It appears that we are trying to run an update during the start of shutdown where the channel AsyncOpen() fails.  This causes the ServiceWorkerScriptCache init to fail.  This should be fine, except ServiceWorkerScriptCache leaks all over the place in this condition.

This patch fixes those leaks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a92c839721ab

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Always use first scheduled update timer instead of rescheduling on new events. r=ehsan

This patch makes us always fire the update timer 1 second after the first event triggers an async update.  This ensures that we always do the update regardless of how many events are being received.

The original P1 patch would reschedule the timer to fire 1 second after the last event is received.  In some ways that is nicer, but it can lead to the update being prevented from firing forever if continuous events are being received.  Given that MessageEvent can be easily triggered programmatically I think we should avoid this possibility.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee79e8923c9e

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Try is looking green.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c85bf5e9bf5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4aff8bbfaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f21ee1cd4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7def186c1d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d518261eb3b1

Comment 45

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c873567c2898 since I didn't have any way to tell whether this was innocent and just didn't save bug 1227015 from its faults, or introduced failures in serviceworker web-platform-tests itself.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Ignore update() called during top level service worker script evaluation. r=ehsan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddb6d18570d7

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

A try build that includes the patch for bug 1213119.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab35b153ec51

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

A new try with some additional patches addressing various wpt test issues.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=831fb5b3acec

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

The try in comment 48 is green across a large number of retriggers.  I'm going to reland.

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e13c56cfb1fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5c858f08b9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd2dccbea6d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9234f07a413a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5be379df35f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/412e32773fbf

Comment 51

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e13c56cfb1fc
https://hg.mozilla.org/mozilla-central/rev/d5c858f08b9d
https://hg.mozilla.org/mozilla-central/rev/cd2dccbea6d5
https://hg.mozilla.org/mozilla-central/rev/9234f07a413a
https://hg.mozilla.org/mozilla-central/rev/5be379df35f1
https://hg.mozilla.org/mozilla-central/rev/412e32773fbf