1472005 - wait for registration state to update before resolving ready promise

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

In IPC mode sometimes the navigator.serviceWorker.ready promise would resolve with a slightly out-of-date registration.  In particular, the registration.active property would be null.  This should be impossible according to the spec.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't resolve ready promise until the registration has reached the right state version. r=mrbkap

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

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8988605 [details] [diff] [review]
Don't resolve ready promise until the registration has reached the right state version. r=mrbkap

Similar to how registration state triggering an updatefound event raced with the update() promise, we also have an issue with registration state changes racing with the navigator.serviceWorker.ready promise.

The ready promise is resolved in step 7.2 here:

https://w3c.github.io/ServiceWorker/#activation-algorithm

This happens after the registration and worker state have been updated.

This can break in IPC mode when the container and registration need to connect to the parent upon creation.  We want to replay the version change list to make sure events are fired correctly, but a site may also immediately access the ready promise.  If the ready promise resolves immediately before the change list has been replayed, then the script may see a resolved ready promise reaction before the registration.active property is set.

This patch fixes the problem by making the .ready code delay resolving the promise until the binding object has reached the correct state version.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8988605 [details] [diff] [review]
Don't resolve ready promise until the registration has reached the right state version. r=mrbkap

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

This is really cool. It's exactly the patch I expected to see.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Try build in comment 1.

Comment 5

[Andreea Pavel [:apavel]]

Patch failed to apply:

mozilla@ubuntu:~/mozilla-unified$ hg qpush
applying bug1472005_ready.patch
patching file dom/serviceworkers/ServiceWorkerRegistration.cpp
Hunk #1 FAILED at 152
Hunk #2 FAILED at 336
2 out of 2 hunks FAILED -- saving rejects to file dom/serviceworkers/ServiceWorkerRegistration.cpp.rej
patching file dom/serviceworkers/ServiceWorkerRegistration.h
Hunk #1 succeeded at 115 with fuzz 1 (offset 0 lines).
Hunk #2 FAILED at 145
1 out of 2 hunks FAILED -- saving rejects to file dom/serviceworkers/ServiceWorkerRegistration.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1472005_ready.patch

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't resolve ready promise until the registration has reached the right state version. r=mrbkap

Rebased

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb950e44164
Don't resolve ready promise until the registration has reached the right state version. r=mrbkap

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4eb950e44164