1402085 - make sure to fire ServiceWorkerContainer.ready promise if the registration is resurrected with an active worker

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

We have a fun bug in our ServiceWorkerContainer.ready promise implementation.  Consider:

  // start with a service worker
  let reg = await navigator.serviceWorker.register(script);
  await waitForState(reg.installing, "activated");

  // get a controlled document to hold the worker alive
  let frame = await withIframe(controlledURL);

  // mark the registration as uninstalling, but it won't
  // fully uninstall because of the frame
  await reg.unregister();
  
  // get the ready promise while in the uninstalling state
  let readyPromise = navigator.serviceWorker.ready;

  // register the service worker again, resurrecting the currently
  // active worker
  await navigator.serviceWorker.register(script);

  // this will never resolve
  await readyPromise;

When we resurrect the registration we should re-check our ready promises.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I wrote a glitch test:

https://sw-ready-resurrection.glitch.me/

It seems chrome has the same bug.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Spec issue:

https://github.com/w3c/ServiceWorker/issues/1198

Also, the glitch seems to prevent us from cleaning up windows stuck on the promise.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Maybe fire ServiceWorkerContainer.ready promise when a registration is resurrected. r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a WPT test case verifying ServiceWorkerContainer.ready works when the registration is resurrected. r=asuth

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8910923 [details] [diff] [review]
Maybe fire ServiceWorkerContainer.ready promise when a registration is resurrected. r=asuth

Andrew, this makes us re-check the ready promise list when we resurrect a doomed registration.  I have nominal agreement from a spec editor to go ahead and fix this:

https://github.com/w3c/ServiceWorker/issues/1198

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8910941 [details] [diff] [review]
P2 Add a WPT test case verifying ServiceWorkerContainer.ready works when the registration is resurrected. r=asuth

Add a WPT test to check the ready promise behavior when resurrecting a doomed registration.  Note, I use an extra timeout mechanism here to avoid long timeouts in other brower test infrastructure.  My initial testing suggests chrome also fails this test and will hang without the manual timeout here.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8910941 [details] [diff] [review]
P2 Add a WPT test case verifying ServiceWorkerContainer.ready works when the registration is resurrected. r=asuth

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

::: testing/web-platform/tests/service-workers/service-worker/ready.https.html
@@ +275,5 @@
> +    let reg2 = await service_worker_unregister_and_register(t, url, matched_scope);
> +    assert_equals(reg1, reg2, 'existing registration should be resurrected');
> +
> +    let timeoutId;
> +    let timeoutPromise = new Promise(resolve => {

I'd suggest adding some variant on your explanation here, like:

On browsers where this test fails, it fails by hanging.  Rather than depend on the test runner to time out after a (long) duration, we create a shorter timeout.  If the test erroneously fails on your platform because your test machine is very slow, it's okay to increase this timeout value upwards.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a WPT test case verifying ServiceWorkerContainer.ready works when the registration is resurrected. r=asuth

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfeb08fc0e90
P1 Maybe fire ServiceWorkerContainer.ready promise when a registration is resurrected. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9755ed7caf71
P2 Add a WPT test case verifying ServiceWorkerContainer.ready works when the registration is resurrected. r=asuth

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/dfeb08fc0e90
https://hg.mozilla.org/mozilla-central/rev/9755ed7caf71

Comment 11

[rok.spacapan89]

(In reply to Ben Kelly [:bkelly, not reviewing] from comment #1)

I wrote a glitch test:

https://sw-ready-resurrection.glitch.me/

It seems chrome has the same bug.

Hey, I added this line
sw.skipWaiting();
as you hava in your glitch,
and activation finally works!

Thanks!