Service Worker: in Unregister, wait until after the active worker no longer controls a document

content/browser/service_worker/service_worker_register_job.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_register_job.h"
 
 #include <vector>
 
 #include "base/message_loop/message_loop.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
@@ skipping 65 matching lines @@
   ServiceWorkerStorage::FindRegistrationCallback next_step;
   if (job_type_ == REGISTRATION_JOB) {
     next_step = base::Bind(
         &ServiceWorkerRegisterJob::ContinueWithRegistration,
         weak_factory_.GetWeakPtr());
   } else {
     next_step = base::Bind(
         &ServiceWorkerRegisterJob::ContinueWithUpdate,
         weak_factory_.GetWeakPtr());
   }
-  context_->storage()->FindRegistrationForPattern(pattern_, next_step);
+
+  scoped_refptr<ServiceWorkerRegistration> registration =
+      context_->storage()->GetUninstallingRegistration(pattern_);
+  if (registration)
+    RunSoon(base::Bind(next_step, SERVICE_WORKER_OK, registration));
+  else
+    context_->storage()->FindRegistrationForPattern(pattern_, next_step);
 }
 
 void ServiceWorkerRegisterJob::Abort() {
   SetPhase(ABORT);
   CompleteInternal(SERVICE_WORKER_ERROR_ABORT);
   // Don't have to call FinishJob() because the caller takes care of removing
   // the jobs from the queue.
 }
 
 bool ServiceWorkerRegisterJob::Equals(ServiceWorkerRegisterJobBase* job) {
@@ skipping 59 matching lines @@
       break;
     case COMPLETE:
       DCHECK(phase_ != INITIAL && phase_ != COMPLETE) << phase_;
       break;
     case ABORT:
       break;
   }
   phase_ = phase;
 }
 
-// This function corresponds to the steps in Register following
-// "Let serviceWorkerRegistration be _GetRegistration(scope)"
-// |existing_registration| corresponds to serviceWorkerRegistration.
+// This function corresponds to the steps in [[Register]] following
+// "Let registration be the result of running the [[GetRegistration]] algorithm.
 // Throughout this file, comments in quotes are excerpts from the spec.
 void ServiceWorkerRegisterJob::ContinueWithRegistration(
     ServiceWorkerStatusCode status,
     const scoped_refptr<ServiceWorkerRegistration>& existing_registration) {
   DCHECK_EQ(REGISTRATION_JOB, job_type_);
-  // On unexpected error, abort this registration job.
   if (status != SERVICE_WORKER_ERROR_NOT_FOUND && status != SERVICE_WORKER_OK) {
     Complete(status);
     return;
   }
 
-  // "If serviceWorkerRegistration is not null and script is equal to
-  // serviceWorkerRegistration.scriptUrl..." resolve with the existing
-  // registration and abort.
-  if (existing_registration.get() &&
-      existing_registration->script_url() == script_url_) {
+  if (!existing_registration) {
+    RegisterAndContinue(SERVICE_WORKER_OK);
+    return;
+  }
+
+  // "1. Set registration.[[Uninstalling]] to false."

[nhiroki, 2014/08/11 12:24:51]

"1. Set ..." -> "2. Set ..."

+  existing_registration->AbortPendingClear();
+
+  // "2. If scriptURL is equal to registration.[[ScriptURL]], then:"

[nhiroki, 2014/08/11 12:24:51]

"1. If scriptURL" would be correct.

This has the different item level from the previous one. To avoid confusion, how
about removing the number, or adding parents number? (eg. "8.1. If scriptURL
...") Wdyt?

[falken, 2014/08/12 09:06:06]

Good catches about the spec quotes. We're currently missing some steps which
causes the quotes to be rather chaotic and unordered. I intend to make the
function more clearly follow the spec in a future patch for
http://crbug.com/398355

For now, I've removed the numbering.

+  if (existing_registration->script_url() == script_url_) {
+    // Spec says to resolve with registration.[[GetNewestWorker]]. We come close
+    // by resolving with the active version.
     set_registration(existing_registration);
-    // If there's no active version, go ahead to Update (this isn't in the spec
-    // but seems reasonable, and without SoftUpdate implemented we can never
-    // Update otherwise).
+
     if (!existing_registration->active_version()) {
       UpdateAndContinue();
       return;
     }
+
     ResolvePromise(
         status, existing_registration, existing_registration->active_version());
     Complete(SERVICE_WORKER_OK);
     return;
   }
 
-  // "If serviceWorkerRegistration is null..." create a new registration.
-  if (!existing_registration.get()) {
-    RegisterAndContinue(SERVICE_WORKER_OK);
-    return;
-  }
-
-  // On script URL mismatch, "set serviceWorkerRegistration.scriptUrl to
-  // script." We accomplish this by deleting the existing registration and
-  // registering a new one.
-  // TODO(falken): Match the spec. We now throw away the active_version_ and
-  // waiting_version_ of the existing registration, which isn't in the spec.
+  // "Set registration.[[ScriptURL]] to scriptURL." We accomplish this by
+  // deleting the existing registration and registering a new one.
   // TODO(michaeln): Deactivate the live existing_registration object and
-  // eventually call storage->DeleteVersionResources()
-  // when it no longer has any controllees.
+  // eventually call storage->DeleteVersionResources() when it no longer has any
+  // controllees.
   context_->storage()->DeleteRegistration(
       existing_registration->id(),
       existing_registration->script_url().GetOrigin(),
       base::Bind(&ServiceWorkerRegisterJob::RegisterAndContinue,
                  weak_factory_.GetWeakPtr()));
 }
 
 void ServiceWorkerRegisterJob::ContinueWithUpdate(
     ServiceWorkerStatusCode status,
     const scoped_refptr<ServiceWorkerRegistration>& existing_registration) {
@@ skipping 297 matching lines @@
   DCHECK(context);
   for (scoped_ptr<ServiceWorkerContextCore::ProviderHostIterator> it =
            context->GetProviderHostIterator();
        !it->IsAtEnd(); it->Advance()) {
     ServiceWorkerProviderHost* host = it->GetProviderHost();
     host->UnsetVersion(version);
   }
 }
 
 }  // namespace content