Service Worker: Handle same-scope, new script registration

content/browser/service_worker/service_worker_register_job.cc

Index: content/browser/service_worker/service_worker_register_job.cc
diff --git a/content/browser/service_worker/service_worker_register_job.cc b/content/browser/service_worker/service_worker_register_job.cc
index a0453313bc0d33e636ac973cd616178b878e28a5..0e16729b69be3da64733c0bdc64918129c302a67 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -140,6 +140,18 @@ ServiceWorkerVersion* ServiceWorkerRegisterJob::new_version() {
   return internal_.new_version;
 }
 
+void ServiceWorkerRegisterJob::set_uninstalling_registration(
+    ServiceWorkerRegistration* registration) {
+  DCHECK_EQ(phase_, WAIT_FOR_UNINSTALL);
+  internal_.uninstalling_registration = registration;
+}
+
+ServiceWorkerRegistration*
+ServiceWorkerRegisterJob::uninstalling_registration() {
+  DCHECK_EQ(phase_, WAIT_FOR_UNINSTALL);
+  return internal_.uninstalling_registration;
+}
+
 void ServiceWorkerRegisterJob::SetPhase(Phase phase) {
   switch (phase) {
     case INITIAL:
@@ -148,9 +160,12 @@ void ServiceWorkerRegisterJob::SetPhase(Phase phase) {
     case START:
       DCHECK(phase_ == INITIAL) << phase_;
       break;
-    case REGISTER:
+    case WAIT_FOR_UNINSTALL:
       DCHECK(phase_ == START) << phase_;
       break;
+    case REGISTER:
+      DCHECK(phase_ == START || phase_ == WAIT_FOR_UNINSTALL) << phase_;
+      break;
     case UPDATE:
       DCHECK(phase_ == START || phase_ == REGISTER) << phase_;
       break;
@@ -186,36 +201,34 @@ void ServiceWorkerRegisterJob::ContinueWithRegistration(
     return;
   }
 
-  // "Set registration.[[Uninstalling]] to false."
-  existing_registration->AbortPendingClear();
-
   // "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 (!existing_registration->active_version()) {
-      UpdateAndContinue();
-      return;
-    }
+    // "Set registration.[[Uninstalling]] to false."
+    existing_registration->AbortPendingClear(base::Bind(
+        &ServiceWorkerRegisterJob::ContinueWithRegistrationForSameScriptUrl,
+        weak_factory_.GetWeakPtr(),
+        existing_registration));
+    return;
+  }
 
-    ResolvePromise(
-        status, existing_registration, existing_registration->active_version());
-    Complete(SERVICE_WORKER_OK);
+  if (existing_registration->is_uninstalling()) {
+    // "Wait until the Record {[[key]], [[value]]} entry of its
+    // [[ScopeToRegistrationMap]] where registation.scope matches entry.[[key]]
+    // is deleted."
+    WaitForUninstall(existing_registration);
     return;
   }
 
-  // "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.
-  context_->storage()->DeleteRegistration(
-      existing_registration->id(),
-      existing_registration->script_url().GetOrigin(),
-      base::Bind(&ServiceWorkerRegisterJob::RegisterAndContinue,
-                 weak_factory_.GetWeakPtr()));
+  // "Set registration.[[ScriptURL]] to scriptURL."
+  existing_registration->set_script_url(script_url_);

[michaeln, 2014/08/21 23:52:44]

This can put the object in a strange state where .active, .installing, and
.waiting can have different script urls. Up till now scripturl has been an
invariant, is anything depending on that invariant> If this job fails (suppose
newScriptUrl is 404s), does this class get left in an incoherent state?

reg.url == newScript (in memory but not on disk)
reg.activeVersion.url == oldScript
there is no waiting or installing version and there is no job running

Maybe reg shouldn't have a scripturl datamember and instead the accessor should
reflects the url of its newest version?

[falken, 2014/08/22 11:08:00]

Thanks for taking a look!

On 2014/08/21 23:52:44, michaeln wrote:
Yep, I'm trying to support .active, .installing, .waiting with different
script URLs.
That's a very good idea, and I think it's what the spec intended. I commented
on GitHub for that 404 case.

I created a patch to remove script URL from SWRegistration
outright, it should land before attempting this patch:
https://codereview.chromium.org/501453002/

+
+  // "Set registration.[[Uninstalling]] to false."
+  DCHECK(!existing_registration->is_uninstalling());
+
+  // "Return the result of running the [[Update]] algorithm, or its equivalent,
+  // passing registration as the argument."
+  set_registration(existing_registration);
+  UpdateAndContinue();
 }
 
 void ServiceWorkerRegisterJob::ContinueWithUpdate(
@@ -256,6 +269,34 @@ void ServiceWorkerRegisterJob::RegisterAndContinue(
   UpdateAndContinue();
 }
 
+void ServiceWorkerRegisterJob::WaitForUninstall(
+    const scoped_refptr<ServiceWorkerRegistration>& existing_registration) {
+  SetPhase(WAIT_FOR_UNINSTALL);
+  set_uninstalling_registration(existing_registration);
+  uninstalling_registration()->AddListener(this);
+}
+
+void ServiceWorkerRegisterJob::ContinueWithRegistrationForSameScriptUrl(
+    const scoped_refptr<ServiceWorkerRegistration>& existing_registration,
+    ServiceWorkerStatusCode status) {
+  if (status != SERVICE_WORKER_OK) {
+    Complete(status);
+    return;
+  }
+  set_registration(existing_registration);
+
+  // TODO(falken): Follow the spec steps. Resolve if the newest version
+  // shares the script URL.
+  if (!existing_registration->active_version()) {
+    UpdateAndContinue();
+    return;
+  }
+
+  ResolvePromise(
+      status, existing_registration, existing_registration->active_version());
+  Complete(SERVICE_WORKER_OK);
+}
+
 // This function corresponds to the spec's [[Update]] algorithm.
 void ServiceWorkerRegisterJob::UpdateAndContinue() {
   SetPhase(UPDATE);
@@ -445,6 +486,15 @@ bool ServiceWorkerRegisterJob::OnMessageReceived(const IPC::Message& message) {
   return false;
 }
 
+void ServiceWorkerRegisterJob::OnRegistrationFinishedUninstalling(
+    ServiceWorkerRegistration* existing_registration) {
+  DCHECK_EQ(phase_, WAIT_FOR_UNINSTALL);
+  DCHECK_EQ(existing_registration, uninstalling_registration());
+  existing_registration->RemoveListener(this);
+  set_uninstalling_registration(NULL);
+  RegisterAndContinue(SERVICE_WORKER_OK);
+}
+
 void ServiceWorkerRegisterJob::OnCompareScriptResourcesComplete(
     ServiceWorkerVersion* most_recent_version,
     ServiceWorkerStatusCode status,