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 96 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 62 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."
+  existing_registration->AbortPendingClear();

[michaeln, 2014/07/29 03:33:39]

what if extisting.script != new.script?

[falken, 2014/07/29 05:36:22]

Great catch, this is a big point. There are two cases to handle:
1) Installation of the new worker fails. To conform to the spec, the
registration should be left untouched (except .installing is nulled).
2) Installation of the new worker succeeds. To conform to the spec, the new
worker becomes the waiting worker for the registration.

Regardless of unregister, I don't think we do 1) or 2) correctly today. In 1) we
delete the registration and in 2) the new worker immediately proceeds to
activation.

I think we need the live registrations to be able to mutate script url over its
lifetime. Or else, when we create a new registration set its active version to
the deleted registration's active version.

Since we don't handle these correctly, I'm inclined to proceed with this patch
as long as there's reasonable behavior and fix for spec compliance in future
patches. I'll write some tests and see.

+
+  // "2. If scriptURL is equal to registration.[[ScriptURL]], then:"
+  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 308 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