Service Worker: Release controllees upon activation failure.

This implements steps added in:
https://github.com/slightlyoff/ServiceWorker/issues/659

Previously, a provider host would hold on to a failed controller, which could
cause a flaky crash when a ServiceWorkerFetchDispatcher is created and expects
a non-null active version.

BUG=471616
TEST=https://codereview.chromium.org/1044123002/

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

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

7 files changed, 59 insertions, 31 deletions

diff --git a/content/browser/service_worker/service_worker_controllee_request_handler.cc b/content/browser/service_worker/service_worker_controllee_request_handler.cc
index 4e29734..ac27aa3 100644
--- a/content/browser/service_worker/service_worker_controllee_request_handler.cc
+++ b/content/browser/service_worker/service_worker_controllee_request_handler.cc
@@ -239,7 +239,8 @@ ServiceWorkerControlleeRequestHandler::DidLookupRegistrationForMainResource(
 
   ServiceWorkerMetrics::CountControlledPageLoad(stripped_url_);
 
-  provider_host_->AssociateRegistration(registration.get());
+  provider_host_->AssociateRegistration(registration.get(),
+                                        false /* notify_controllerchange */);
   job_->ForwardToServiceWorker();
   TRACE_EVENT_ASYNC_END2(
       "ServiceWorker",
@@ -264,7 +265,8 @@ void ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged(
 
   ServiceWorkerMetrics::CountControlledPageLoad(stripped_url_);
 
-  provider_host_->AssociateRegistration(registration);
+  provider_host_->AssociateRegistration(registration,
+                                        false /* notify_controllerchange */);
   job_->ForwardToServiceWorker();
 }
 
diff --git a/content/browser/service_worker/service_worker_provider_host.cc b/content/browser/service_worker/service_worker_provider_host.cc
index 9dc0ba2..648d55a 100644
--- a/content/browser/service_worker/service_worker_provider_host.cc
+++ b/content/browser/service_worker/service_worker_provider_host.cc
@@ -83,8 +83,7 @@ ServiceWorkerProviderHost::ServiceWorkerProviderHost(
       provider_type_(provider_type),
       context_(context),
       dispatcher_host_(dispatcher_host),
-      allow_association_(true),
-      is_claiming_(false) {
+      allow_association_(true) {
   DCHECK_NE(ChildProcessHost::kInvalidUniqueID, render_process_id_);
   DCHECK_NE(SERVICE_WORKER_PROVIDER_UNKNOWN, provider_type_);
   if (provider_type_ == SERVICE_WORKER_PROVIDER_FOR_CONTROLLER) {
@@ -151,7 +150,8 @@ void ServiceWorkerProviderHost::OnSkippedWaiting(
     return;
   ServiceWorkerVersion* active_version = registration->active_version();
   DCHECK_EQ(active_version->status(), ServiceWorkerVersion::ACTIVATING);
-  SetControllerVersionAttribute(active_version);
+  SetControllerVersionAttribute(active_version,
+                                true /* notify_controllerchange */);
 }
 
 void ServiceWorkerProviderHost::SetDocumentUrl(const GURL& url) {
@@ -164,7 +164,8 @@ void ServiceWorkerProviderHost::SetTopmostFrameUrl(const GURL& url) {
 }
 
 void ServiceWorkerProviderHost::SetControllerVersionAttribute(
-    ServiceWorkerVersion* version) {
+    ServiceWorkerVersion* version,
+    bool notify_controllerchange) {
   if (version == controlling_version_.get())
     return;
 
@@ -178,15 +179,11 @@ void ServiceWorkerProviderHost::SetControllerVersionAttribute(
   if (!dispatcher_host_)
     return;  // Could be NULL in some tests.
 
-  bool should_notify_controllerchange =
-      is_claiming_ || (previous_version && version && version->skip_waiting());
-
   // SetController message should be sent only for controllees.
   DCHECK_NE(SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, provider_type_);
   Send(new ServiceWorkerMsg_SetControllerServiceWorker(
       render_thread_id_, provider_id(),
-      CreateAndRegisterServiceWorkerHandle(version),
-      should_notify_controllerchange));
+      CreateAndRegisterServiceWorkerHandle(version), notify_controllerchange));
 }
 
 bool ServiceWorkerProviderHost::SetHostedVersionId(int64 version_id) {
@@ -229,12 +226,14 @@ blink::WebServiceWorkerClientType ServiceWorkerProviderHost::client_type()
 }
 
 void ServiceWorkerProviderHost::AssociateRegistration(
-    ServiceWorkerRegistration* registration) {
+    ServiceWorkerRegistration* registration,
+    bool notify_controllerchange) {
   DCHECK(CanAssociateRegistration(registration));
   associated_registration_ = registration;
   AddMatchingRegistration(registration);
   SendAssociateRegistrationMessage();
-  SetControllerVersionAttribute(registration->active_version());
+  SetControllerVersionAttribute(registration->active_version(),
+                                notify_controllerchange);
 }
 
 void ServiceWorkerProviderHost::DisassociateRegistration() {
@@ -242,7 +241,7 @@ void ServiceWorkerProviderHost::DisassociateRegistration() {
   if (!associated_registration_.get())
     return;
   associated_registration_ = NULL;
-  SetControllerVersionAttribute(NULL);
+  SetControllerVersionAttribute(NULL, false /* notify_controllerchange */);
 
   if (!dispatcher_host_)
     return;
@@ -289,6 +288,10 @@ ServiceWorkerProviderHost::MatchRegistration() const {
   return nullptr;
 }
 
+void ServiceWorkerProviderHost::NotifyControllerActivationFailed() {
+  SetControllerVersionAttribute(nullptr, true /* notify_controllerchange */);
+}
+
 scoped_ptr<ServiceWorkerRequestHandler>
 ServiceWorkerProviderHost::CreateRequestHandler(
     FetchRequestMode request_mode,
@@ -410,14 +413,13 @@ void ServiceWorkerProviderHost::AddScopedProcessReferenceToPattern(
 void ServiceWorkerProviderHost::ClaimedByRegistration(
     ServiceWorkerRegistration* registration) {
   DCHECK(registration->active_version());
-  is_claiming_ = true;
   if (registration == associated_registration_) {
-    SetControllerVersionAttribute(registration->active_version());
+    SetControllerVersionAttribute(registration->active_version(),
+                                  true /* notify_controllerchange */);
   } else if (allow_association_) {
     DisassociateRegistration();
-    AssociateRegistration(registration);
+    AssociateRegistration(registration, true /* notify_controllerchange */);
   }
-  is_claiming_ = false;
 }
 
 bool ServiceWorkerProviderHost::GetRegistrationForReady(
diff --git a/content/browser/service_worker/service_worker_provider_host.h b/content/browser/service_worker/service_worker_provider_host.h
index e624de70..b8de167 100644
--- a/content/browser/service_worker/service_worker_provider_host.h
+++ b/content/browser/service_worker/service_worker_provider_host.h
@@ -108,8 +108,11 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   ServiceWorkerProviderType provider_type() const { return provider_type_; }
   blink::WebServiceWorkerClientType client_type() const;
 
-  // Associates to |registration| to listen for its version change events.
-  void AssociateRegistration(ServiceWorkerRegistration* registration);
+  // Associates to |registration| to listen for its version change events and
+  // sets the controller. If |notify_controllerchange| is true, instructs the
+  // renderer to dispatch a 'controllerchange' event.
+  void AssociateRegistration(ServiceWorkerRegistration* registration,
+                             bool notify_controllerchange);
 
   // Clears the associated registration and stop listening to it.
   void DisassociateRegistration();
@@ -214,6 +217,8 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   // for current document.
   ServiceWorkerRegistration* MatchRegistration() const;
 
+  void NotifyControllerActivationFailed();
+
  private:
   friend class ServiceWorkerProviderHostTest;
   friend class ServiceWorkerWriteToCacheJobTest;
@@ -242,8 +247,10 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   void OnSkippedWaiting(ServiceWorkerRegistration* registration) override;
 
   // Sets the controller version field to |version| or if |version| is NULL,
-  // clears the field.
-  void SetControllerVersionAttribute(ServiceWorkerVersion* version);
+  // clears the field. If |notify_controllerchange| is true, instructs the
+  // renderer to dispatch a 'controller' change event.
+  void SetControllerVersionAttribute(ServiceWorkerVersion* version,
+                                     bool notify_controllerchange);
 
   void SendAssociateRegistrationMessage();
 
@@ -281,7 +288,6 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   base::WeakPtr<ServiceWorkerContextCore> context_;
   ServiceWorkerDispatcherHost* dispatcher_host_;
   bool allow_association_;
-  bool is_claiming_;
 
   std::vector<base::Closure> queued_events_;
 
diff --git a/content/browser/service_worker/service_worker_provider_host_unittest.cc b/content/browser/service_worker/service_worker_provider_host_unittest.cc
index 6139644..79a8ac16 100644
--- a/content/browser/service_worker/service_worker_provider_host_unittest.cc
+++ b/content/browser/service_worker/service_worker_provider_host_unittest.cc
@@ -101,7 +101,8 @@ TEST_F(ServiceWorkerProviderHostTest, PotentialRegistration_ProcessStatus) {
 TEST_F(ServiceWorkerProviderHostTest, AssociatedRegistration_ProcessStatus) {
   // Associating the registration will also increase the process refs for
   // the registration's pattern.
-  provider_host1_->AssociateRegistration(registration1_.get());
+  provider_host1_->AssociateRegistration(registration1_.get(),
+                                         false /* notify_controllerchange */);
   ASSERT_TRUE(PatternHasProcessToRun(registration1_->pattern()));
 
   // Disassociating the registration shouldn't affect the process refs for
diff --git a/content/browser/service_worker/service_worker_registration.cc b/content/browser/service_worker/service_worker_registration.cc
index eef79b4..26d4528 100644
--- a/content/browser/service_worker/service_worker_registration.cc
+++ b/content/browser/service_worker/service_worker_registration.cc
@@ -298,11 +298,26 @@ void ServiceWorkerRegistration::OnActivateEventFinished(
     ServiceWorkerStatusCode status) {
   if (!context_ || activating_version != active_version())
     return;
-  // TODO(kinuko,falken): For some error cases (e.g. ServiceWorker is
-  // unexpectedly terminated) we may want to retry sending the event again.
+  // "If activateFailed is true, then:..."
   if (status != SERVICE_WORKER_OK) {
-    // "11. If activateFailed is true, then:..."
+    // "Set registration's active worker to null." (The spec's step order may
+    // differ. It's OK because the other steps queue a task.)
     UnsetVersion(activating_version);
+
+    // "Run the Update State algorithm passing registration's active worker and
+    // 'redundant' as the arguments."
+    activating_version->SetStatus(ServiceWorkerVersion::REDUNDANT);
+
+    // "For each service worker client client whose active worker is
+    // registration's active worker..." set the active worker to null.
+    for (scoped_ptr<ServiceWorkerContextCore::ProviderHostIterator> it =
+             context_->GetProviderHostIterator();
+         !it->IsAtEnd(); it->Advance()) {
+      ServiceWorkerProviderHost* host = it->GetProviderHost();
+      if (host->controlling_version() == activating_version)
+        host->NotifyControllerActivationFailed();
+    }
+
     activating_version->Doom();
     if (!waiting_version()) {
       // Delete the records from the db.
@@ -320,8 +335,8 @@ void ServiceWorkerRegistration::OnActivateEventFinished(
     return;
   }
 
-  // "12. Run the [[UpdateState]] algorithm passing registration.activeWorker
-  // and "activated" as the arguments."
+  // "Run the Update State algorithm passing registration's active worker and
+  // 'activated' as the arguments."
   activating_version->SetStatus(ServiceWorkerVersion::ACTIVATED);
   if (context_) {
     context_->storage()->UpdateToActiveState(
diff --git a/content/browser/service_worker/service_worker_request_handler_unittest.cc b/content/browser/service_worker/service_worker_request_handler_unittest.cc
index f21f37e..4ab7994 100644
--- a/content/browser/service_worker/service_worker_request_handler_unittest.cc
+++ b/content/browser/service_worker/service_worker_request_handler_unittest.cc
@@ -66,7 +66,8 @@ class ServiceWorkerRequestHandlerTest : public testing::Test {
         registration_.get(),
         version_.get(),
         base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
-    provider_host_->AssociateRegistration(registration_.get());
+    provider_host_->AssociateRegistration(registration_.get(),
+                                          false /* notify_controllerchange */);
     base::RunLoop().RunUntilIdle();
   }
 
diff --git a/content/browser/service_worker/service_worker_url_request_job_unittest.cc b/content/browser/service_worker/service_worker_url_request_job_unittest.cc
index ff666643..1f3dd48 100644
--- a/content/browser/service_worker/service_worker_url_request_job_unittest.cc
+++ b/content/browser/service_worker/service_worker_url_request_job_unittest.cc
@@ -156,7 +156,8 @@ class ServiceWorkerURLRequestJobTest : public testing::Test {
                                       helper_->context()->AsWeakPtr(),
                                       nullptr));
     provider_host->SetDocumentUrl(GURL("http://example.com/"));
-    provider_host->AssociateRegistration(registration_.get());
+    provider_host->AssociateRegistration(registration_.get(),
+                                         false /* notify_controllerchange */);
     registration_->SetActiveVersion(version_.get());
 
     ChromeBlobStorageContext* chrome_blob_storage_context =