Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should the worker be removed from the registration *before* its state is set to "redundant"? #1273

Open
cdumez opened this issue Feb 2, 2018 · 10 comments · May be fixed by #1416
Open

Should the worker be removed from the registration *before* its state is set to "redundant"? #1273

cdumez opened this issue Feb 2, 2018 · 10 comments · May be fixed by #1416
Assignees

Comments

@cdumez
Copy link

cdumez commented Feb 2, 2018

While investigating some web-platform-tests failures on WebKit, a possible difference in behavior between Firefox/Chrome and the specification.

For example, when the install step fails, the spec says:
If installFailed is true, then:

  1. Run the Update Worker State algorithm passing registration’s installing worker and redundant as the arguments.
  2. Run the Update Registration State algorithm passing registration, "installing" and null as the arguments.

Notice the order, we first set the worker state to redundant, then we set the registration's installing worker to null. This means that when the statechange event is fired on this worker, registration.installing is not set to null yet (according to my understanding of the spec).

However, local testing shows that in Firefox and Chrome, registration.installing already returns null when the statechange event is fired at the worker (with state being redundant).

Should the specification be updated to match the behavior of browsers? Or did I misinterpret the specification?

I believe there is a similar issue in the "Clear Registration" algorithm. There is a WPT test that seems to assume that when clearing the registration, and the waiting worker becomes redundant, then all the registration's installing/waiting/active worker should be null. This test seems to pass in Chrome/Firefox.
However, my understanding is that the spec says:

  1. Set installing worker state to redundant & schedule a task to fire statechange event at worker
  2. Schedule a task to update ServiceWorkerRegistration(s) installing worker to null
  3. Set waiting worker state to redundant & schedule a task to fire statechange event at worker
  4. Schedule a task to update ServiceWorkerRegistration(s) waiting worker to null
  5. Set active worker state to redundant & schedule a task to fire statechange event at worker
  6. Schedule a task to update ServiceWorkerRegistration(s) active worker to null

So when the statechange is fired at the waiting worker, based on the spec, I would not expect registration.waiting / registration.active to become null right away. Instead, I would have expected registration.waiting to return null after 1 event loop iteration, and registration.active even later.

@wanderview
Copy link
Member

Is this the same as #1270? (I was just looking at this yesterday as well.)

@mfalken
Copy link
Member

mfalken commented Feb 2, 2018

I think your reading of the spec is correct.

For the "Clear Registration" test, are you talking about registration-service-worker-attributes.https.html? I suspect the comment there was written for an older version of the spec:

          // According to spec, Clear Registration runs Update State which is
          // immediately followed by setting active to null, which means by the
          // time the event loop turns and the Promise for statechange is
          // resolved, this will be gone.

(And yea I think can dupe this to #1270 or vice versa)

@wanderview
Copy link
Member

I think we should reverse the order of updating in the spec. The registration should be updated before we fire the statechange event, IMO.

@mfalken
Copy link
Member

mfalken commented Feb 2, 2018

Yes, I tend to think that when the worker is "redundant" it should be dead and gone from the registration. I don't know if there's a particular reason the spec ended up with this other order. It seems like the intent was "redundant" means the worker is "going away" based on the non-normative note here: https://w3c.github.io/ServiceWorker/#ref-for-dom-serviceworkerstate-redundant%E2%91%A0

@wanderview
Copy link
Member

The spec originally didn't have any task queuing to update the DOM objects. I think the order just got reversed on this one call site when that was added.

@cdumez
Copy link
Author

cdumez commented Feb 2, 2018

Well there are at least 2 call sites, Clear and Install (when it fails).

@mfalken
Copy link
Member

mfalken commented Feb 6, 2018

I'm OK with changing the spec then, especially since it sounds like this order wasn't intended.

@asakusuma
Copy link

I've seen what looks like the same root issue, where registration.active.state === 'redundant'. I've been able to reproduce this issue intermittently in headless chrome service worker tests.

@jakearchibald jakearchibald self-assigned this Jun 4, 2019
@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 4, 2019

As a general rule, we should ensure that the following properties are updated:

  • installing/waiting/active on registration objects.
  • state on service worker objects.
  • controller on navigator.serviceWorker.

…before notifying the developer that any of those things have changed. Our current notifications are:

  • updatefound event on registration objects.
  • statechange event on service worker objects.
  • navigator.serviceWorker.ready.
  • controllerchange event on navigator.serviceWorker.
  • install, activate event on the service worker global.

I'll review the spec for this, and ideally come up with a system that enforces the above.

@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 14, 2019

Some research on what the spec says vs what browsers do:

One worker, going through the steps

When I say "reg: null, null, worker" I mean, on the registration, installing was null, waiting was null, active was a worker:

on register resolve() updatefound after resolve on "installed" on "activating" on "activated"
Spec reg: worker, null, null Yes. reg: worker, null, null reg: null, worker, null reg: null, null, worker reg: null, null, worker

Chrome, Firefox and Safari all behave the same as the spec.

An new active worker

This is where things are different. I'm going to compare:

In this test, worker1 is active, worker2 is waiting. worker2 becomes active, worker1 becomes redundant.

Event order when worker1 "redundant" when worker2 "activating"
Spec redundant, activating worker2 "installed"
reg: null, worker2, worker1
worker1 "redundant"
reg: null, null, worker2
#1416 redundant, activating worker2 "activating"
reg: null, null, worker2
worker1 "redundant"
reg: null, null, worker2
#1436 redundant, activating worker2 "installed"
reg: null, null, worker2
worker1 "redundant"
reg: null, null, worker2
Chrome redundant, activating worker2 "installed"
reg: null, worker2, worker1
worker1 "redundant"
reg: null, null, worker2
Safari redundant, activating worker2 "installed"
reg: null, worker2, worker1
worker1 "redundant"
reg: null, null, worker2
Firefox activating, redundant worker2 "activating"
reg: null, null, worker2
worker1 "redundant"
reg: null, null, worker2

Chrome and Safari match the current spec, but that means a redundant worker is seen within the registration.

Firefox gets the order wrong.

A new waiting worker

In this test, worker1 is active, worker2 is waiting, worker3 is installing. Then, worker3 becomes waiting, worker 2 becomes redundant.

The story is the same as above.

Clear registration

In this test, worker1 is active, worker2 is waiting. The registration is unregistered.

Redundancy order when worker2 "redundant" when worker1 "redundant"
Spec worker2, worker1 worker1 "activated"
worker2 "redundant"
reg: null, worker2, worker1
worker1 "redundant"
worker2 "redundant"
reg: null, null, worker1
#1416 worker2, worker1 worker1 "redundant"
worker2 "redundant"
reg: null, null, null
worker1 "redundant"
worker2 "redundant"
reg: null, null, null
#1436 worker2, worker1 worker1 "activated"
worker2 "redundant"
reg: null, null, null
worker1 "redundant"
worker2 "redundant"
reg: null, null, null
Chrome worker2, worker1 worker1 "activated"
worker2 "redundant"
reg: null, null, null
worker1 "redundant"
worker2 "redundant"
reg: null, null, null
Safari worker2, worker1 worker1 "activated"
worker2 "redundant"
reg: null, null, null
worker1 "redundant"
worker2 "redundant"
reg: null, null, null
Firefox worker2, worker1 worker1 "redundant"
worker2 "redundant"
reg: null, null, null
worker1 "redundant"
worker2 "redundant"
reg: null, null, null

Here the browsers break from the spec (I think they're following a test) and update the registration before firing any statechange events. Firefox goes further and updates both workers before firing any events.

I think our options are:

  • Spec what Chrome and Safari are doing.
    • In some cases the registration will be updated before the statechange events fire, sometimes they won't.
    • Within a transaction, 'statechange' happens on the youngest worker first.
    • Within a transaction, worker.state is updated just before that worker's 'statechange' (meaning other workers will be showing old values)
  • Update order #1436
    • For a transaction, update the registration before firing any 'statechange' events.
    • Within a transaction, 'statechange' happens on the youngest worker first.
    • Within a transaction, worker.state is updated just before that worker's 'statechange' (meaning other workers will be showing old values).
  • Central algorithm for updating service worker state and registrations #1416
    • For a transaction, update the registration before firing any 'statechange' events.
    • For a transaction, update all worker.state properties before firing any 'statechange' events.
    • Within a transaction, 'statechange' happens on the youngest worker first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants