why does Register algorithm step 5.3.1 check for an active worker?

[wanderview]

Currently the Register algorithm does the following:

5. If registration is not null, then:
    1. If registration’s uninstalling flag is set, unset it.
    2. Let newestWorker be the result of running the Get Newest Worker algorithm
        passing registration as the argument.
    3. If newestWorker is not null and job’s script url equals newestWorker’s script url, then:
        1. If newestWorker is an active worker, then:
            1. Invoke Resolve Job Promise with job and the ServiceWorkerRegistration object
                which represents registration.
            2. Invoke Finish Job with job and abort these steps.

The check for active worker in 5.3.1 here surprises me. Why do we want to perform the network request if the newest worker matches our script URL, but is still in the process of installing/waiting?

I see that this was added in this commit, but the comment there doesn't really help me understand. I'm not sure the update step it refers to exists any more (or its been moved).

5f9b752

[wanderview]

To clarify, we don't implement this active worker check. Does chrome?

[mfalken]

Chrome's implementation seems to predate the spec here, and it does a check but it's slightly different: if there's no active worker, then continue to Update. Nothing about the newest worker.

I think I just wanted to avoid register() triggering an update when the registration was already settled. If there's no active worker, then it's considered unsettled. Our code has a comment:

  // "If newestWorker is not null, and scriptURL is equal to
  // newestWorker.scriptURL, then:
  // Return a promise resolved with registration."
  // We resolve only if there's an active version. If there's not,
  // then there is either no version or only a waiting version from
  // the last browser session; it makes sense to proceed with registration in
  // either case.
  DCHECK(!existing_registration->installing_version());
  if (existing_registration->active_version()) {
    ResolvePromise(status, std::string(), existing_registration.get());
    Complete(SERVICE_WORKER_OK);
    return;
  }

The comment mentions "no version" registrations, which should be no longer possible.

Does Firefox early exit in all cases?

[wanderview]

We early exit if there is a newest worker and the script URL matches. We don't check the state of the newest worker or if there is an active worker.

I guess I'm not opposed to the check for active, but want to understand why.

With the current unified queue spec language I think the only non-active state that could match would be an installed .waiting worker. Why would we want .register() to re-download the script for a .waiting with same script URL, but not a .active?

[mfalken]

I guess proceeding with update could allow the waiting worker or soon-to-be installed worker to become .active, in case someone does register().then(wait for an active version).

However, Chrome's current impl looks it'll only do that if the new worker is different than the waiting worker.

[jakearchibald]

Pre F2F notes: What's the behaviour change if we drop that line?

[jakearchibald]

F2F:

• The intent is for register to be a no-op if the URL is the same, so we can remove this line if it maintains that

[jungkees]

@mattto @nhiroki - please note that we confirmed during the f2f the intention is as described in 8ae780f. I noticed Chromium still has the same code shown in #856 (comment). It'd be necessary to address this change.