1226441 - Service Worker functional events should wait for activate to complete

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Cătălin Badea (:catalinb)]

In gecko, activate jobs are not synchronized with a queue as described in the spec. This means we can have racing activating service workers. This is not a problem at the moment since we don't wait for the worker to finish activating in Handle Fetch either, but it might be worth investigating later on.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

By coincidence bdahl noticed this makes things racey for web developers.  For example, they should be able to count on the .ready promise resolving before the navigation-triggered updatefound event.  But they can't because we don't wait for the activated state.

Catalin, how much work would this be?  I think it would be worth uplifting to 44, although I wouldn't block the release on it.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Another reason to wait on the activate event is that this event is often used for upgrading IDB schemas.  For example, see Jake's answer here:

  http://stackoverflow.com/questions/29206836/accessing-indexeddb-in-serviceworker-race-condition

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

We should talk about this in our next triage.  I'm worried about production sites breaking because their fetch events fire before their IDB is updated in the activate event.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I'm going to see what we can do for this today.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

A couple spec issues I saw related to this:

  https://github.com/slightlyoff/ServiceWorker/issues/785
  https://github.com/slightlyoff/ServiceWorker/issues/786

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add wpt test verifying fetch event waits for activate to complete.

Here is a wpt test that demonstrates the problem.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Delay functional event dispatch until service worker is activated. r=catalinb

This implements the waiting behavior for both fetch and push events.  It depends on the patches in bug 1189659.

Note, I explicitly don't do anything for activation queueing here.  I split that off into bug 1226749.  I expect that will be somewhat more rare, requiring two windows and a skipWaiting() service worker.

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8690110 [details] [diff] [review]
P1 Add wpt test verifying fetch event waits for activate to complete.

Test passes with P2 patch.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Note, the M(4) errors are due to a race exposed by bug 1189659.  There is a P4 patch there to fix the test, but I didn't include it in this try.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Another try with that missing patch:

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

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Also, note, the patches here follow the spec for waiting-for-activated whether or not we have a queue.  The spec text in Handle Fetch does do anything with the queue at all.  It just looks at the currently active worker regardless of any further activation queue logic.

Comment 12

[Cătălin Badea (:catalinb)]

Comment on attachment 8690275 [details] [diff] [review]
P2 Delay functional event dispatch until service worker is activated. r=catalinb

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

lgtm

::: dom/workers/ServiceWorkerManager.cpp
@@ +4594,5 @@
>  #endif
> +  // Flush any pending functional events to the worker when it transitions to the
> +  // activated state.
> +  // TODO: Do we care that these events will race with the propagation of the
> +  //       state change?

I don't think so, but we should probably ask the spec folks.

Comment 13

[(no longer active)]

Catalin, this needs to be uplifted, right?  Do you mind asking for approval since you are more familiar with the changes, please?

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0823da696ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/671bc38157d2

Comment 15

[Cătălin Badea (:catalinb)]

Comment on attachment 8690110 [details] [diff] [review]
P1 Add wpt test verifying fetch event waits for activate to complete.

Approval Request Comment
(Uplift request for both patches)
[Feature/regressing bug #]: Service Workers
[User impact if declined]: These patches fix an important race condition related to how SW handle functional events.
[Describe test coverage new/current, TreeHerder]: web platform tests.
[Risks and why]: Minimal. Service Workers specific change.
[String/UUID change made/needed]: None

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d0823da696ba
https://hg.mozilla.org/mozilla-central/rev/671bc38157d2

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8690110 [details] [diff] [review]
P1 Add wpt test verifying fetch event waits for activate to complete.

test-only change, approved.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8690275 [details] [diff] [review]
P2 Delay functional event dispatch until service worker is activated. r=catalinb

SW is planned for 44, taking it.

Comment 19

[Carsten Book [:Tomcat]]

part2 failed to apply to aurora:

grafting 316548:671bc38157d2 "Bug 1226441 - Part 2: Delay functional event dispatch until service worker is activated; r=catalinb"
merging dom/workers/ServiceWorkerManager.cpp
merging dom/workers/ServiceWorkerManager.h
merging dom/workers/ServiceWorkerPrivate.cpp
merging dom/workers/ServiceWorkerPrivate.h
warning: conflicts while merging dom/workers/ServiceWorkerPrivate.h! (edit, then use 'hg resolve --mark')

can you take a look, thanks!

Comment 20

[Cătălin Badea (:catalinb)]

Landed the patches on aurora:
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bab785163ce4
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7ea5152b8417

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bab785163ce4
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7ea5152b8417