Service Worker functional events should wait for activate to complete

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: catalinb, Assigned: bkelly)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 wontfix, firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 1226443
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.
Flags: needinfo?(catalin.badea392)
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
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.
Blocks: ServiceWorkers-v1
No longer blocks: ServiceWorkers-postv1
I'm going to see what we can do for this today.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(catalin.badea392)
Here is a wpt test that demonstrates the problem.
(Assignee)

Updated

4 years ago
Summary: Service Workers don't use an activation queue → Service Worker functional events should wait for activate to complete
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
Attachment #8690275 - Flags: review?(catalin.badea392)
Comment on attachment 8690110 [details] [diff] [review]
P1 Add wpt test verifying fetch event waits for activate to complete.

Test passes with P2 patch.
Attachment #8690110 - Flags: review?(catalin.badea392)
(Assignee)

Updated

4 years ago
Attachment #8690275 - Attachment description: bug1226441_p2_waitforactivate.patch → P2 Delay functional event dispatch until service worker is activated. r=catalinb
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.
(Assignee)

Updated

4 years ago
Depends on: 1189659
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.
(Reporter)

Updated

4 years ago
Attachment #8690110 - Flags: review?(catalin.badea392) → review+
(Reporter)

Comment 12

4 years ago
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.
Attachment #8690275 - Flags: review?(catalin.badea392) → review+
Catalin, this needs to be uplifted, right?  Do you mind asking for approval since you are more familiar with the changes, please?
Flags: needinfo?(catalin.badea392)
(Reporter)

Comment 15

4 years ago
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
Flags: needinfo?(catalin.badea392)
Attachment #8690110 - Flags: approval-mozilla-aurora?

Comment 16

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d0823da696ba
https://hg.mozilla.org/mozilla-central/rev/671bc38157d2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8690110 [details] [diff] [review]
P1 Add wpt test verifying fetch event waits for activate to complete.

test-only change, approved.
Attachment #8690110 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8690275 - Flags: approval-mozilla-aurora+
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!
Flags: needinfo?(bkelly)
Flags: needinfo?(bkelly) → needinfo?(catalin.badea392)
You need to log in before you can comment on or make changes to this bug.