service worker: Add instrumentation to investigate invalid controller status

There seems to be a situation where ServiceWorkerProviderHost::controller()
returns a redundant ServiceWorkerVersion. This CL adds instrumentation
code to investigate the situation.

Bug: 951571
Change-Id: Ic07395a6cebef8056a18e8b030a4baba68cd6de7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1577322
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653413}

content/browser/service_worker/service_worker_context_unittest.cc

@@ -375,6 +375,9 @@ TEST_F(ServiceWorkerContextTest, NoControlleesObserver) {
   auto version = base::MakeRefCounted<ServiceWorkerVersion>(
       registration.get(), script_url, blink::mojom::ScriptType::kClassic,
       2l /* dummy version id */, context()->AsWeakPtr());
+  version->set_fetch_handler_existence(
+      ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
+  version->SetStatus(ServiceWorkerVersion::ACTIVATED);
 
   ServiceWorkerRemoteProviderEndpoint endpoint;
   base::WeakPtr<ServiceWorkerProviderHost> host =

content/browser/service_worker/service_worker_provider_host.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
+#include "base/debug/alias.h"
 #include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
@@ -309,6 +310,11 @@ bool ServiceWorkerProviderHost::IsContextSecureForServiceWorker() const {
   return schemes.find(url_.scheme()) != schemes.end();
 }
 
+ServiceWorkerVersion* ServiceWorkerProviderHost::controller() const {
+  CheckControllerConsistency();
+  return controller_.get();
+}
+
 blink::mojom::ControllerServiceWorkerMode
 ServiceWorkerProviderHost::GetControllerMode() const {
   if (!controller_)
@@ -357,10 +363,10 @@ void ServiceWorkerProviderHost::OnSkippedWaiting(
   if (controller_registration_ != registration)
     return;
 
-  DCHECK(controller());
+  DCHECK(controller_);
   ServiceWorkerVersion* active = controller_registration_->active_version();
   DCHECK(active);
-  DCHECK_NE(active, controller());
+  DCHECK_NE(active, controller_.get());
   DCHECK_EQ(active->status(), ServiceWorkerVersion::ACTIVATING);
   UpdateController(true /* notify_controllerchange */);
 }
@@ -843,7 +849,6 @@ bool ServiceWorkerProviderHost::IsControllerDecided() const {
   return true;
 }
 
-#if DCHECK_IS_ON()
 void ServiceWorkerProviderHost::CheckControllerConsistency() const {
   if (!controller_) {
     DCHECK(!controller_registration_);
@@ -852,8 +857,23 @@ void ServiceWorkerProviderHost::CheckControllerConsistency() const {
   DCHECK(IsProviderForClient());
   DCHECK(controller_registration_);
   DCHECK_EQ(controller_->registration_id(), controller_registration_->id());
+  switch (controller_->status()) {
+    case ServiceWorkerVersion::NEW:
+    case ServiceWorkerVersion::INSTALLING:
+    case ServiceWorkerVersion::INSTALLED:
+    case ServiceWorkerVersion::REDUNDANT: {
+      ServiceWorkerVersion::Status status = controller_->status();
+      base::debug::Alias(&status);
+      CHECK(false) << "Controller service worker has a bad status: "
+                   << ServiceWorkerVersion::VersionStatusToString(status);
+      break;
+    }
+    case ServiceWorkerVersion::ACTIVATING:
+    case ServiceWorkerVersion::ACTIVATED:
+      // Valid status, controller is being activated.
+      break;
+  }
 }
-#endif
 
 void ServiceWorkerProviderHost::Register(
     const GURL& script_url,

content/browser/service_worker/service_worker_provider_host.h

@@ -180,13 +180,7 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   blink::mojom::ControllerServiceWorkerMode GetControllerMode() const;
 
   // For service worker clients. Returns this client's controller.
-  ServiceWorkerVersion* controller() const {
-#if DCHECK_IS_ON()
-    CheckControllerConsistency();
-#endif  // DCHECK_IS_ON()
-
-    return controller_.get();
-  }
+  ServiceWorkerVersion* controller() const;
 
   ServiceWorkerRegistration* controller_registration() const {
 #if DCHECK_IS_ON()
@@ -514,9 +508,9 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   // controller has not yet been decided.
   bool IsControllerDecided() const;
 
-#if DCHECK_IS_ON()
+  // TODO(crbug.com/951571): Put this check function behind DCHECK_IS_ON() once
+  // we figured out the cause of invalid controller status.
   void CheckControllerConsistency() const;
-#endif  // DCHECK_IS_ON()
 
   // Implements blink::mojom::ServiceWorkerContainerHost.
   void Register(const GURL& script_url,

content/browser/service_worker/service_worker_registration_unittest.cc

@@ -1078,8 +1078,11 @@ TEST_F(ServiceWorkerRegistrationObjectHostTest,
   const GURL kScriptUrl("https://www.example.com/sw.js");
   scoped_refptr<ServiceWorkerRegistration> registration =
       CreateRegistration(kScope);
+
   scoped_refptr<ServiceWorkerVersion> version =
       CreateVersion(registration.get(), kScriptUrl);
+  version->SetStatus(ServiceWorkerVersion::ACTIVATED);
+
   ServiceWorkerRemoteProviderEndpoint remote_endpoint;
   base::WeakPtr<ServiceWorkerProviderHost> host = CreateProviderHostForWindow(
       helper_->mock_render_process_id(), true /* is_parent_frame_secure */,

content/browser/service_worker/service_worker_storage_unittest.cc

@@ -1465,6 +1465,7 @@ TEST_F(ServiceWorkerResourceStorageTest, DeleteRegistration_WaitingVersion) {
 TEST_F(ServiceWorkerResourceStorageTest, DeleteRegistration_ActiveVersion) {
   // Promote the worker to active and add a controllee.
   registration_->SetActiveVersion(registration_->waiting_version());
+  registration_->active_version()->SetStatus(ServiceWorkerVersion::ACTIVATED);
   storage()->UpdateToActiveState(registration_.get(), base::DoNothing());
   ServiceWorkerRemoteProviderEndpoint remote_endpoint;
   base::WeakPtr<ServiceWorkerProviderHost> host = CreateProviderHostForWindow(
@@ -1512,6 +1513,7 @@ TEST_F(ServiceWorkerResourceStorageTest, DeleteRegistration_ActiveVersion) {
 TEST_F(ServiceWorkerResourceStorageDiskTest, CleanupOnRestart) {
   // Promote the worker to active and add a controllee.
   registration_->SetActiveVersion(registration_->waiting_version());
+  registration_->active_version()->SetStatus(ServiceWorkerVersion::ACTIVATED);
   registration_->SetWaitingVersion(nullptr);
   storage()->UpdateToActiveState(registration_.get(), base::DoNothing());
   ServiceWorkerRemoteProviderEndpoint remote_endpoint;
@@ -1672,6 +1674,7 @@ TEST_F(ServiceWorkerResourceStorageDiskTest,
 TEST_F(ServiceWorkerResourceStorageTest, UpdateRegistration) {
   // Promote the worker to active worker and add a controllee.
   registration_->SetActiveVersion(registration_->waiting_version());
+  registration_->active_version()->SetStatus(ServiceWorkerVersion::ACTIVATED);
   storage()->UpdateToActiveState(registration_.get(), base::DoNothing());
   ServiceWorkerRemoteProviderEndpoint remote_endpoint;
   base::WeakPtr<ServiceWorkerProviderHost> host = CreateProviderHostForWindow(

content/browser/service_worker/service_worker_version.cc

@@ -702,6 +702,9 @@ void ServiceWorkerVersion::AddControllee(
   const std::string& uuid = provider_host->client_uuid();
   CHECK(!provider_host->client_uuid().empty());
   DCHECK(!base::ContainsKey(controllee_map_, uuid));
+  // TODO(crbug.com/951571): Change to DCHECK once we figured out the cause of
+  // invalid controller status.
+  CHECK(status_ == ACTIVATING || status_ == ACTIVATED);
 
   controllee_map_[uuid] = provider_host;
   embedded_worker_->UpdateForegroundPriority();