(Reland) Service Worker: Handle same-scope, new script registration

Reland of 2da192af after:
- disabling some layout tests that need to be rebaselined
- landing 6cce6b78f "Decouple script_url from ServiceWorkerRegistration"
so Registration doesn't get in a weird state with a script_url that failed to
register

Before this patch, register() would delete an existing registration at
the scope if the script URL didn't match, and register a new one. This
overwriting creates a scenario where old tabs have a different
controller than new tabs, which the Service Worker spec avoids.

This patch implements the spec steps for same-scope, new script
register(). That means:
- If the existing registration is uninstalling, wait for that to
complete before doing anything.
- Create a new worker which becomes the installing worker of the
existing registration.

BUG=398355
TEST=https://codereview.chromium.org/480943002/

Review URL: https://codereview.chromium.org/506043002

Cr-Commit-Position: refs/heads/master@{#292106}

content/browser/service_worker/service_worker_context_unittest.cc

@@ -300,8 +300,8 @@ TEST_F(ServiceWorkerContextTest, Unregister) {
   base::RunLoop().RunUntilIdle();
 }
 
-// Make sure that when a new registration replaces an existing
-// registration, that the old one is cleaned up.
+// Make sure registering a new script creates a new version and shares an
+// existing registration.
 TEST_F(ServiceWorkerContextTest, RegisterNewScript) {
   GURL pattern("http://www.example.com/");
 
@@ -335,11 +335,9 @@ TEST_F(ServiceWorkerContextTest, RegisterNewScript) {
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(called);
 
-  // Returned IDs should be valid, and should differ from the values
-  // returned for the previous registration.
   EXPECT_NE(kInvalidServiceWorkerRegistrationId, new_registration_id);
   EXPECT_NE(kInvalidServiceWorkerVersionId, new_version_id);
-  EXPECT_NE(old_registration_id, new_registration_id);
+  EXPECT_EQ(old_registration_id, new_registration_id);
   EXPECT_NE(old_version_id, new_version_id);
 }
 

content/browser/service_worker/service_worker_job_unittest.cc

@@ -288,8 +288,8 @@ TEST_F(ServiceWorkerJobTest, Unregister_NothingRegistered) {
   ASSERT_TRUE(called);
 }
 
-// Make sure that when a new registration replaces an existing
-// registration, that the old one is cleaned up.
+// Make sure registering a new script creates a new version and shares an
+// existing registration.
 TEST_F(ServiceWorkerJobTest, RegisterNewScript) {
   GURL pattern("http://www.example.com/");
 
@@ -329,9 +329,7 @@ TEST_F(ServiceWorkerJobTest, RegisterNewScript) {
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(called);
 
-  ASSERT_TRUE(old_registration->HasOneRef());
-
-  ASSERT_NE(old_registration, new_registration);
+  ASSERT_EQ(old_registration, new_registration);
 
   scoped_refptr<ServiceWorkerRegistration> new_registration_by_pattern;
   storage()->FindRegistrationForPattern(

content/browser/service_worker/service_worker_provider_host.h

@@ -40,7 +40,7 @@ class ServiceWorkerVersion;
 // Note this class can also host a running service worker, in which
 // case it will observe resource loads made directly by the service worker.
 class CONTENT_EXPORT ServiceWorkerProviderHost
-    : public ServiceWorkerRegistration::Listener,
+    : public NON_EXPORTED_BASE(ServiceWorkerRegistration::Listener),
       public base::SupportsWeakPtr<ServiceWorkerProviderHost> {
  public:
   ServiceWorkerProviderHost(int process_id,

content/browser/service_worker/service_worker_register_job.cc

@@ -140,6 +140,18 @@ ServiceWorkerVersion* ServiceWorkerRegisterJob::new_version() {
   return internal_.new_version.get();
 }
 
+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,37 +201,31 @@ void ServiceWorkerRegisterJob::ContinueWithRegistration(
     return;
   }
 
-  // "Set registration.[[Uninstalling]] to false."
-  existing_registration->AbortPendingClear();
-
   // "If scriptURL is equal to registration.[[ScriptURL]], then:"
   if (existing_registration->GetNewestVersion()->script_url() == script_url_) {
-    // Spec says to resolve with registration.[[GetNewestWorker]]. We come close
-    // by resolving with the active version.
-    set_registration(existing_registration.get());
-
-    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.get(),
-                   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->pattern().GetOrigin(),
-      base::Bind(&ServiceWorkerRegisterJob::RegisterAndContinue,
-                 weak_factory_.GetWeakPtr()));
+  // "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(
@@ -264,6 +273,35 @@ 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: resolve the promise
+  // with the newest version.
+
+  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);
@@ -455,6 +493,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,

content/browser/service_worker/service_worker_register_job.h

@@ -33,9 +33,9 @@ class ServiceWorkerStorage;
 //  - waiting for older ServiceWorkerVersions to deactivate
 //  - designating the new version to be the 'active' version
 //  - updating storage
-class ServiceWorkerRegisterJob
-    : public ServiceWorkerRegisterJobBase,
-      public EmbeddedWorkerInstance::Listener {
+class ServiceWorkerRegisterJob : public ServiceWorkerRegisterJobBase,
+                                 public EmbeddedWorkerInstance::Listener,
+                                 public ServiceWorkerRegistration::Listener {
  public:
   typedef base::Callback<void(ServiceWorkerStatusCode status,
                               ServiceWorkerRegistration* registration,
@@ -74,14 +74,15 @@ class ServiceWorkerRegisterJob
                            DisassociateVersionFromDocuments);
 
   enum Phase {
-     INITIAL,
-     START,
-     REGISTER,
-     UPDATE,
-     INSTALL,
-     STORE,
-     COMPLETE,
-     ABORT,
+    INITIAL,
+    START,
+    WAIT_FOR_UNINSTALL,
+    REGISTER,
+    UPDATE,
+    INSTALL,
+    STORE,
+    COMPLETE,
+    ABORT,
   };
 
   // Holds internal state of ServiceWorkerRegistrationJob, to compel use of the
@@ -94,12 +95,16 @@ class ServiceWorkerRegisterJob
     // Holds the version created by this job. It can be the 'installing',
     // 'waiting', or 'active' version depending on the phase.
     scoped_refptr<ServiceWorkerVersion> new_version;
+
+    scoped_refptr<ServiceWorkerRegistration> uninstalling_registration;
   };
 
   void set_registration(ServiceWorkerRegistration* registration);
   ServiceWorkerRegistration* registration();
   void set_new_version(ServiceWorkerVersion* version);
   ServiceWorkerVersion* new_version();
+  void set_uninstalling_registration(ServiceWorkerRegistration* registration);
+  ServiceWorkerRegistration* uninstalling_registration();
 
   void SetPhase(Phase phase);
 
@@ -110,6 +115,11 @@ class ServiceWorkerRegisterJob
       ServiceWorkerStatusCode status,
       const scoped_refptr<ServiceWorkerRegistration>& registration);
   void RegisterAndContinue(ServiceWorkerStatusCode status);
+  void WaitForUninstall(
+      const scoped_refptr<ServiceWorkerRegistration>& registration);
+  void ContinueWithRegistrationForSameScriptUrl(
+      const scoped_refptr<ServiceWorkerRegistration>& existing_registration,
+      ServiceWorkerStatusCode status);
   void UpdateAndContinue();
   void OnStartWorkerFinished(ServiceWorkerStatusCode status);
   void OnStoreRegistrationComplete(ServiceWorkerStatusCode status);
@@ -127,6 +137,10 @@ class ServiceWorkerRegisterJob
   virtual void OnPausedAfterDownload() OVERRIDE;
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
+  // ServiceWorkerRegistration::Listener overrides
+  virtual void OnRegistrationFinishedUninstalling(
+      ServiceWorkerRegistration* registration) OVERRIDE;
+
   void OnCompareScriptResourcesComplete(
       ServiceWorkerVersion* most_recent_version,
       ServiceWorkerStatusCode status,

content/browser/service_worker/service_worker_registration.cc

@@ -167,10 +167,13 @@ void ServiceWorkerRegistration::ClearWhenReady() {
     Clear();
 }
 
-void ServiceWorkerRegistration::AbortPendingClear() {
+void ServiceWorkerRegistration::AbortPendingClear(
+    const StatusCallback& callback) {
   DCHECK(context_);
-  if (!is_uninstalling())
+  if (!is_uninstalling()) {
+    callback.Run(SERVICE_WORKER_OK);
     return;
+  }
   is_uninstalling_ = false;
   context_->storage()->NotifyDoneUninstallingRegistration(this);
 
@@ -181,8 +184,9 @@ void ServiceWorkerRegistration::AbortPendingClear() {
   context_->storage()->StoreRegistration(
       this,
       most_recent_version.get(),
-      base::Bind(&ServiceWorkerRegistration::OnStoreFinished,
+      base::Bind(&ServiceWorkerRegistration::OnRestoreFinished,
                  this,
+                 callback,
                  most_recent_version));
 }
 
@@ -278,7 +282,9 @@ void ServiceWorkerRegistration::OnDeleteFinished(
 }
 
 void ServiceWorkerRegistration::Clear() {
-  context_->storage()->NotifyDoneUninstallingRegistration(this);
+  is_uninstalling_ = false;
+  if (context_)
+    context_->storage()->NotifyDoneUninstallingRegistration(this);
 
   ChangedVersionAttributesMask mask;
   if (installing_version_.get()) {
@@ -302,15 +308,22 @@ void ServiceWorkerRegistration::Clear() {
     FOR_EACH_OBSERVER(Listener, listeners_,
                       OnVersionAttributesChanged(this, mask, info));
   }
+
+  FOR_EACH_OBSERVER(
+      Listener, listeners_, OnRegistrationFinishedUninstalling(this));
 }
 
-void ServiceWorkerRegistration::OnStoreFinished(
+void ServiceWorkerRegistration::OnRestoreFinished(
+    const StatusCallback& callback,
     scoped_refptr<ServiceWorkerVersion> version,
     ServiceWorkerStatusCode status) {
-  if (!context_)
+  if (!context_) {
+    callback.Run(ServiceWorkerStatusCode::SERVICE_WORKER_ERROR_ABORT);
     return;
+  }
   context_->storage()->NotifyDoneInstallingRegistration(
       this, version.get(), status);
+  callback.Run(status);
 }
 
 }  // namespace content

content/browser/service_worker/service_worker_registration.h

@@ -20,10 +20,9 @@ namespace content {
 class ServiceWorkerRegistrationInfo;
 class ServiceWorkerVersion;
 
-// This class represents a service worker registration. The
-// scope is constant for the life of the persistent
-// registration. It's refcounted to facillitate multiple controllees
-// being associated with the same registration.
+// This class represents a Service Worker registration. The scope is constant
+// for the life of the persistent registration. It's refcounted to facilitate
+// multiple controllees being associated with the same registration.
 class CONTENT_EXPORT ServiceWorkerRegistration
     : NON_EXPORTED_BASE(public base::RefCounted<ServiceWorkerRegistration>),
       public ServiceWorkerVersion::Listener {
@@ -35,9 +34,11 @@ class CONTENT_EXPORT ServiceWorkerRegistration
     virtual void OnVersionAttributesChanged(
         ServiceWorkerRegistration* registration,
         ChangedVersionAttributesMask changed_mask,
-        const ServiceWorkerRegistrationInfo& info) = 0;
+        const ServiceWorkerRegistrationInfo& info) {}
     virtual void OnRegistrationFailed(
-        ServiceWorkerRegistration* registration) = 0;
+        ServiceWorkerRegistration* registration) {}
+    virtual void OnRegistrationFinishedUninstalling(
+        ServiceWorkerRegistration* registration) {}
   };
 
   ServiceWorkerRegistration(const GURL& pattern,
@@ -95,9 +96,8 @@ class CONTENT_EXPORT ServiceWorkerRegistration
   void ClearWhenReady();
 
   // Restores this registration in storage and cancels the pending
-  // [[ClearRegistration]] algorithm. If the algorithm was already triggered,
-  // does nothing.
-  void AbortPendingClear();
+  // [[ClearRegistration]] algorithm.
+  void AbortPendingClear(const StatusCallback& callback);
 
   // The time of the most recent update check.
   base::Time last_update_check() const { return last_update_check_; }
@@ -128,8 +128,10 @@ class CONTENT_EXPORT ServiceWorkerRegistration
 
   // This method corresponds to the [[ClearRegistration]] algorithm.
   void Clear();
-  void OnStoreFinished(scoped_refptr<ServiceWorkerVersion> version,
-                       ServiceWorkerStatusCode status);
+
+  void OnRestoreFinished(const StatusCallback& callback,
+                         scoped_refptr<ServiceWorkerVersion> version,
+                         ServiceWorkerStatusCode status);
 
   const GURL pattern_;
   const int64 registration_id_;