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

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

57 Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
I wrote a glitch test:

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

It seems chrome has the same bug.
Spec issue:

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

Also, the glitch seems to prevent us from cleaning up windows stuck on the promise.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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
Attachment #8910923 - Flags: review?(bugmail)
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.
Attachment #8910941 - Flags: review?(bugmail)
Attachment #8910923 - Flags: review?(bugmail) → review+
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.
Attachment #8910941 - Flags: review?(bugmail) → review+

Comment 9

2 years ago
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/dfeb08fc0e90
https://hg.mozilla.org/mozilla-central/rev/9755ed7caf71
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.