The promises returned by skipWaiting() should have more consistent activation-waiting behavior and not call Activate() multiple times

[asutherland]

The current high-level intent for ServiceWorkerGlobalScope.skipWaiting() seems to be:

• If the SW is handling an "install" event (state = "installing"), set the skip waiting flag and resolve the promise after the flag is set. But don't wait for activation because that would deadlock any install handler that does event.waitUntil(self.skipWaiting()).
• If the SW has been installed (state = "installed") and calls skipWaiting(), set the skip waiting flag and wait for activation to complete; the promise will not resolve until the SW has been made active. This happens in cases like the WPT skip-waiting-installed.https.html test where skipWaiting() is invoked in response to a "message" event.

As specified right now, in the latter case if you call skipWaiting() twice, the first call will work as expected (set skip waiting, invoke Activate() and not resolve until Activate() fully completes). If you call it a second time, Activate() will be invoked again, aborting on step 1 ("If registration’s waiting worker is null, abort these steps.") because the previous Activate() call will have transitioned the worker to be the registration's "active worker". The promise will then resolve immediately regardless of whether the SW has activated or not.

There is also a potential variant on the second skipWaiting() call case where "Handle Service Worker Client Unload" calling Activate() races against the SW calling skipWaiting() at the request of an uncontrolled client's postMessage. If skipWaiting() wins, the promise waits for activation. If it loses, it doesn't wait.

Per discussion with @wanderview we think the core of a fix to make skipWaiting()'s promise consistent would be to:

• Only invoke Activate() if the skip waiting flag was not already set.
• Keep a list of all of the skipWaiting promises handed out (skipWaiting is annotated with [NewObject]) that are waiting on activation and resolve them when the SW activates, even if it's via a different Activate() invocation.
• Ensure the promise will always be resolved even if the skipWaiting() call is made after the SW is already active.

One way to do accomplish this might be to:

• Add verbiage to service worker: "A service worker has an associated list of skip waiting promises whose element type is a promise. It is initially empty."
• Rewrite skipWaiting() to be:
  • Let promise be a new promise.
  • Run the following substeps in parallel:
    • If service worker's state is not installed and not activating, then resolve promise with undefined and abort these steps.
    • Add promise to the service worker's list of pending skip waiting promises.
    • If service worker's skip waiting flag is set then abort these steps.
    • Set service worker's skip waiting flag.
    • Run Activate algorithm passing service worker's registration as the argument.
  • Return promise.
• Add the following step to the end of Activate.
  • Resolve the promises in active worker's skip waiting promise list with undefined and clear the list.

[jungkees]

If you call it a second time, Activate() will be invoked again, aborting on step 1 ("If registration’s waiting worker is null, abort these steps.") because the previous Activate() call will have transitioned the worker to be the registration's "active worker". The promise will then resolve immediately regardless of whether the SW has activated or not.

"Handle Service Worker Client Unload" calling Activate() races against the SW calling skipWaiting() at the request of an uncontrolled client's postMessage.

Right. It seems they obviously need to be fixed. Your suggestion would be an option here, but let me clarify two things around the desired behavior and the race condition.

First, when called on a waiting worker, do we want skipWaiting() promise to not resolve until the Activate is complete? This seems like a special case comparing to other cases where skipWaiting() just resolves after setting the flag (and waiting for it to happen.) One usage I can think of with the current behavior is:

self.onmessage = e => {
  e.waitUntil(
    self.skipWaiting().then(() => {
      e.source.postMessage('Activated.');
    })
  );
};

But I think the client already has other ways to detect the registration has an active worker that can control it such as navigator.serviceWorker.ready and registration.active.onstatechange. I'm not sure if it has other compelling usages. Please let me know if you've encountered any.

That said, I think it'd give more consistent and expected behavior to devs if skipWaiting() would resolve early even in the case that the state is "installed". Activate, of course, continues in parallel.

Secondly, having reviewed the steps again, Activate (currently cannot-be-queued job) and Unregister (queued job) can race on registration's waiting worker, I suppose. As far as I remember, this was previously pointed out by @wanderview. Sorry about that. I was worrying about it possibly blocking the Register/Update jobs in the job queue while waiting until no client is using the registration. However, I think it was just my confusion. It seems we can create and queue the Activate job outside the waiting condition. I wonder @wanderview still thinks it'd be right decision to make Activate a job to be queued in the job queue. If that's the case, we will probably be able to take advantage of job aggregation as well.

/cc @jakearchibald

[asutherland]

That said, I think it'd give more consistent and expected behavior to devs if skipWaiting() would resolve early even in the case that the state is "installed". Activate, of course, continues in parallel.

Yes, this is a much better idea! It reduces the complexity and reduces skipWaiting() to its core. As you describe it now, I understand that the current spec just wanted to ensure activation was initiated in the case the "Install" algorithm had already completed. Currently step 24 of Install's "Wait until... registration's waiting worker's skip waiting flag is set." phrasing may cover this now? Would the skipWaiting() description just provide a pointer/reference to that?

[jungkees]

Currently step 24 of Install's "Wait until... registration's waiting worker's skip waiting flag is set." phrasing may cover this now?

Yes, I think the case is covered here, but we still need to change the step 25 where Activate is invoked only when "waitingWorker’s skip waiting flag is not set". This condition has been there to avoid redundant calls to Activate as, with the current text, skipWaiting() should have invoked Activate already as well. (Related issue: #594.)

I think we now agree on the expected behavior. I'd like to have @jakearchibald's input before changing this though since he initially spec'd this part. (Just want to confirm what the original intention was and we're not missing any important bits.)

@wanderview, also your feedback about Activate job idea would be helpful. If the implementation is just good without that change, I don't think I want to change it for now. The scenario I'm concerned about is Activate is entered and executed as waiting worker is not null, and while it's executing a queued Unregister job is scheduled, run and reaches Clear Registration interleaved with Activate.

[asutherland]

Test-wise, I audited the tests and found only service-workers/service-worker/skip-waiting-installed.https.html makes any assumptions about skipWaiting() providing guarantees about activation having occurred. All other uses of skipWaiting() separately wait for activation to occur.

[jakearchibald]

That said, I think it'd give more consistent and expected behavior to devs if skipWaiting() would resolve early even in the case that the state is "installed". Activate, of course, continues in parallel.

I agree this is the best thing to do now.

I think if we did everything again, I'd prefer it if skipWaiting always waited until the service worker became activating, but as @asutherland points out there's too much code out there using it in waitUntil, so it'll become a deadlock.

[jungkees]

Test-wise, I audited the tests and found only service-workers/service-worker/skip-waiting-installed.https.html makes any assumptions

@asutherland, thanks for checking it!

/cc @mattto, @nhiroki

[asutherland]

(This was resolved in December by the merged patch, closing.)