Service Worker: Handle same-scope, new script registration

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 122 matching lines @@
   DCHECK(phase_ == UPDATE) << phase_;
   DCHECK(!internal_.new_version);
   internal_.new_version = version;
 }
 
 ServiceWorkerVersion* ServiceWorkerRegisterJob::new_version() {
   DCHECK(phase_ >= UPDATE) << phase_;
   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:
       NOTREACHED();
       break;
     case START:
       DCHECK(phase_ == INITIAL) << phase_;
       break;
+    case WAIT_FOR_UNINSTALL:
+      DCHECK(phase_ == START) << phase_;
+      break;
     case REGISTER:
-      DCHECK(phase_ == START) << phase_;
+      DCHECK(phase_ == START || phase_ == WAIT_FOR_UNINSTALL) << phase_;
       break;
     case UPDATE:
       DCHECK(phase_ == START || phase_ == REGISTER) << phase_;
       break;
     case INSTALL:
       DCHECK(phase_ == UPDATE) << phase_;
       break;
     case STORE:
       DCHECK(phase_ == INSTALL) << phase_;
       break;
@@ skipping 16 matching lines @@
   if (status != SERVICE_WORKER_ERROR_NOT_FOUND && status != SERVICE_WORKER_OK) {
     Complete(status);
     return;
   }
 
   if (!existing_registration) {
     RegisterAndContinue(SERVICE_WORKER_OK);
     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;
-    }
-
-    ResolvePromise(
-        status, existing_registration, existing_registration->active_version());
-    Complete(SERVICE_WORKER_OK);
+    // "Set registration.[[Uninstalling]] to false."
+    existing_registration->AbortPendingClear(base::Bind(
+        &ServiceWorkerRegisterJob::ContinueWithRegistrationForSameScriptUrl,
+        weak_factory_.GetWeakPtr(),
+        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()));
+  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."
+  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(
     ServiceWorkerStatusCode status,
     const scoped_refptr<ServiceWorkerRegistration>& existing_registration) {
   DCHECK_EQ(UPDATE_JOB, job_type_);
   if (status != SERVICE_WORKER_OK) {
     Complete(status);
     return;
   }
@@ skipping 20 matching lines @@
     return;
   }
 
   set_registration(new ServiceWorkerRegistration(
       pattern_, script_url_, context_->storage()->NewRegistrationId(),
       context_));
   AssociateProviderHostsToRegistration(registration());
   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);
   context_->storage()->NotifyInstallingRegistration(registration());
 
   // TODO(falken): "If serviceWorkerRegistration.installingWorker is not null.."
   // then terminate the installing worker. It doesn't make sense to implement
   // yet since we always activate the worker if install completed, so there can
   // be no installing worker at this point.
 
@@ skipping 169 matching lines @@
       most_recent_script_id, new_script_id,
       base::Bind(&ServiceWorkerRegisterJob::OnCompareScriptResourcesComplete,
                  weak_factory_.GetWeakPtr(),
                  most_recent_version));
 }
 
 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,
     bool are_equal) {
   if (are_equal) {
     // Only bump the last check time when we've bypassed the browser cache.
     base::TimeDelta time_since_last_check =
         base::Time::Now() - registration()->last_update_check();
     if (time_since_last_check > base::TimeDelta::FromHours(24)) {
       registration()->set_last_update_check(base::Time::Now());
@@ skipping 19 matching lines @@
     ServiceWorkerProviderHost* host = it->GetProviderHost();
     if (ServiceWorkerUtils::ScopeMatches(registration->pattern(),
                                          host->document_url())) {
       if (host->CanAssociateRegistration(registration))
         host->AssociateRegistration(registration);
     }
   }
 }
 
 }  // namespace content