"Unregister" operation may be interrupted by ServiceWorker#postMessage

[jugglinmike]

Consider the following code:

// Precondition: `registration`is a valid ServiceWorkerRegistration object
// whose "active" worker has no pending events
registration.unregister()
  .then(function() {
      registration.active.postMessage('');
    });

"unregistration" involves the following sequence:

1. Queue a task to resolve the promise (via Resolve Job Promise)
2. If the active worker has no pending events (via Try Clear Registration)
   1. Terminate the active worker (via Clear Registration)
      1. Set the worker's "closing" flag to true
   2. Queue a task to set the state attribute of the active worker to
      "redundant" (via Update Worker State)
   3. Queue a task to set the registration's active property to null
      (via Update Registration State)

This means the state attribute is not set until after the promise has been
resolved and any subsequent microtasks processed. Here, the postMessage
invocation is part of such a microtask, so when it references the worker's
state, it receives the value "active". Execution of the postMessage
algorithm proceeds to step 4, where Run Service Worker is invoked. This creates
a new ServiceWorkerGlobalScope object, effectively undoing the prior invocation
of Terminate Worker.

Chromium's behavior more or less adheres to this interpretation, although the
worker's script is only evaluated one time (I would expect a second evaluation
for that invocation of Run Service Worker). Firefox throws an error named
NS_ERROR_FAILURE from the postMessage invocation. In either case, this
seems like a spec bug--the "unregister" operation is being interrupted, but the
worker state is still ultimately set to "redundant."

Distinguishing between "user-requested un-registration" and "user-agent
requested termination" seems necessary in order to decide whether to throw or
re-start the worker. Making this determination may not be possible from an
arbitrary postMessage invocation without the introduction of additional
state or a modification to the schedule of operations in Clear Registration.

[jungkees]

Thanks for reviewing to this detail. Unregister algorithm being invoked means the update job is scheduled and run. So, we set the uninstalling flag to not allow getting this registration or running any action on it from this point. (Only a scheduled register job can flip this.) Also, we try to clear registration at the earliest possible (via Try Clear Registration in many points where the registration's workers are idle.)

But as you pointed, we missed this right within the Unregister algorithm itself. I think we should invoke Try Clear Registration before invoking Resolve Job Promise. That'll make the resolve task will be scheduled after the tasks that mutate the states. Then, the unregister() promise's settlement callbacks when scheduled will see the active worker became redundant. So, I think the expected behavior here should be postMessage() throw an "InvalidStateError". WDYT?

[wanderview]

I don't think we can safely call Try Clear Registration from the thread calling unregister(). The state checks in Try Clear Registration is managed on a different thread. Also, I'm not sure it would be correct if you consider the different ways a register job could also be queued.

We probably just need algorithms to handle the uninstalling flag. For postMessage() it should probably just eat the message silently as thats what we do when you postMessage a closing window. For other things we may want to throw an error.

We could also add text saying the UA should not start a new SW thread for a registration with the uninstalling flag set.

[jungkees]

I don't think we can safely call Try Clear Registration from the thread calling unregister(). The state checks in Try Clear Registration is managed on a different thread.

Try Clear Registration insn't called from the thread calling unregister(). It's called from a thread scheduling the job queue. Also, since we're updating the states from queued tasks (Update Worker State, Update Registration State), I don't see a problem maintaining the order of changing the states.

Also, I'm not sure it would be correct if you consider the different ways a register job could also be queued.

I'm not seeing a problem here. Let's say a register job R is queued after an unregister job Un. If the Try Clear Registration from Un triggered Clear Regirstration as the registration was totally idle, R won't find the registration in its execution later in the first place. Else if the Try Clear Registration from Un just returned without invoking Clear Registration, R will unset the registration's uninstalling flag.

For postMessage() it should probably just eat the message silently as thats what we do when you postMessage a closing window. For other things we may want to throw an error.

postMessage() to an uninstalling registration's worker (as in the OP example) throwing early sounds more reasonable to me. For other things, e.g. for getRegistration(), Match Service Worker Registration, not returning the result while uninstalling flag is set sounds very reasonable to me too. In my mental model, calling unregister() means I expect no further actions to the registration should happen but admit that I should wait until other clients are all being served.

We could also add text saying the UA should not start a new SW thread for a registration with the uninstalling flag set.

I think this is a good idea. We can add a condition in the begining of Run Service Worker to not allow starting a thread if the given worker's registration is uninstalling.

[wanderview]

I really don't want to alter state outside of the job queue. We used to do that and it had so many race conditions. I think we should work with the uninstalling flag as the given signal.

I'm not sure what the semantics of various calls should be. We should probably talk to @jakearchibald or other web devs about that. Maybe this would be a good one to discuss in person at next meeting.

[jungkees]

Hmm.. I think I got you. The Try Clear Registration initiated from unregister() is called in the updateunregister job in the job queue. So, that'll work. But we have other call sites to Try Clear Registration: event.waitUntil(), event.respondWith(), and Handle Service Worker Client Unload. Is that what you mean?

At least, all of the them check if the registration's uninstalling flag is set before they invoke Try Clear Registration. But that's not enough to avoid them race, right?

I think we should work with the uninstalling flag as the given signal.

Could you get a little more specific on this? I'd like to think about this option together.

We should probably talk to @jakearchibald or other web devs about that. Maybe this would be a good one to discuss in person at next meeting.

Surely though I hope we can find a resolution even before that.

[wanderview]

At least, all of the them check if the registration's uninstalling flag is set before they invoke Try Clear Registration. But that's not enough to avoid them race, right?

I think those places are mostly right for checking the installing flag. The steps that call them probably need to be executed on the thread managing the job queue, though. Right now they are on the worker thread in the spec.

Could you get a little more specific on this? I'd like to think about this option together.

I was just saying methods on the ServiceWorker and ServiceWorkerRegistration objects can probably check the uninstalling flag in order to trigger any "don't allow this on an uninstalling service worker" logic.

Another perspective, though, is that as long as a page has the ServiceWorker DOM object referenced and the service worker is not in the redundant state, then the methods on that object should work. Maybe we should permit postMessage() in this case. If something is listed as registration.active that certainly suggests it should be usable.

With that approach we would base the "don't allow this on an uninstalling service worker" based on the service worker's state (redundant, etc). This would be the least racy, but would allow postMessage() while waiting for the active worker to go idle.

This is where I'm not sure what semantics make most sense for developers.

[mfalken]

For this particular issue I'm thinking:

• worker.postMessage should throw InvaildStateError if worker.state is "redundant"
• If that didn't happen, postMessage should drop silently if the service worker's state (the conceptual service worker represented by |worker|) is redundant before trying to start the worker.
• Invoking Run Service Worker on a conceptual service worker whose state is redundant should fail (or assert).

There are some related issues here like whether postMessage() should block on Run Service Worker (mentioned in #1403 ) and we generally don't really check to see if Run Service Worker failed (with the exception of the Update algorithm).

I'm not sure if the spec changed since the discussion on this thread, but using the registration's uninstalling flag for this doesn't seem right... we set the flag in unregister() but the service worker is still alive and usable while it has clients (so it can stop and be restarted multiple times).

[wanderview]

I feel like postMessage() throwing is somewhat unexpected. For example, we don't do that in cases where the other end of MessageChannel is gone. I would be more in favor of sending the message to /dev/null, maybe with a console warning.

[jakearchibald]

That's a fair point. I guess silent-failing is at least consistent.

[mfalken]

SGTM.

[mfalken]

Addressed in #1419.

[mfalken]

WPT: web-platform-tests/wpt#17246
Chromium: https://crbug.com/972461
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1558098
WebKit: https://bugs.webkit.org/show_bug.cgi?id=198702