service worker: Add futher instrumentation to investigate invalid controller status

Make sure that the previous controller no longer has controllees when
activating a waiting worker if the previous controller exists.

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

content/browser/service_worker/service_worker_registration.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/debug/crash_logging.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/service_worker/embedded_worker_status.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
@@ -408,6 +409,30 @@ void ServiceWorkerRegistration::ActivateWaitingVersion(bool delay) {
       observer.OnSkippedWaiting(this);
   }
 
+  // TODO(crbug.com/951571): Remove this instrumentation logic once the bug is
+  // debugged.
+  if (exiting_version.get()) {
+    int skip_waiting = activating_version->skip_waiting();
+    static auto* key = base::debug::AllocateCrashKeyString(
+        "swv_skip_waiting", base::debug::CrashKeySize::Size32);
+    base::debug::ScopedCrashKeyString(key, base::NumberToString(skip_waiting));
+    CHECK(!exiting_version->HasControllee());
+    for (std::unique_ptr<ServiceWorkerContextCore::ProviderHostIterator> it =
+             context_->GetClientProviderHostIterator(
+                 scope_.GetOrigin(), false /* include_reserved_clients */);
+         !it->IsAtEnd(); it->Advance()) {
+      ServiceWorkerProviderHost* host = it->GetProviderHost();
+      if (!host->IsContextSecureForServiceWorker())
+        continue;
+      if (host->MatchRegistration() != this)
+        continue;
+      ServiceWorkerVersion* controller = host->controller();
+      if (!controller)
+        continue;
+      CHECK(controller != exiting_version);
+    }
+  }
+
   // "10. Queue a task to fire an event named activate..."
   // The browser could be shutting down. To avoid spurious start worker
   // failures, wait a bit before continuing.

content/browser/service_worker/service_worker_registration_unittest.cc

@@ -373,6 +373,7 @@ class ServiceWorkerActivationTest : public ServiceWorkerRegistrationTest,
   void SetUp() override {
     ServiceWorkerRegistrationTest::SetUp();
 
+    const GURL kUrl("https://www.example.not/");
     const GURL kScope("https://www.example.not/");
     const GURL kScript("https://www.example.not/service_worker.js");
 
@@ -414,7 +415,9 @@ class ServiceWorkerActivationTest : public ServiceWorkerRegistrationTest,
         context()->AsWeakPtr(), &remote_endpoint_);
     DCHECK(remote_endpoint_.client_request()->is_pending());
     DCHECK(remote_endpoint_.host_ptr()->is_bound());
-    version_1->AddControllee(host_.get());
+    host_->UpdateUrls(kUrl, kUrl);
+    host_->SetControllerRegistration(registration_,
+                                     false /* notify_controllerchange */);
 
     // Setup the Mojo implementation fakes for the renderer-side service worker.
     // These will be bound once the service worker starts.
@@ -480,6 +483,16 @@ class ServiceWorkerActivationTest : public ServiceWorkerRegistrationTest,
   ServiceWorkerProviderHost* controllee() { return host_.get(); }
   int inflight_request_id() const { return inflight_request_id_; }
 
+  void AddControllee() {
+    controllee()->SetControllerRegistration(
+        registration(), false /* notify_controllerchange */);
+  }
+
+  void RemoveControllee() {
+    controllee()->SetControllerRegistration(
+        nullptr, false /* notify_controllerchange */);
+  }
+
   bool IsLameDuckTimerRunning() {
     return registration_->lame_duck_timer_.IsRunning();
   }
@@ -549,7 +562,7 @@ TEST_P(ServiceWorkerActivationTest, NoInflightRequest) {
 
   // Remove the controllee. Since there is an in-flight request,
   // activation should not yet happen.
-  version_1->RemoveControllee(controllee()->client_uuid());
+  RemoveControllee();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(version_1.get(), reg->active_version());
   // The idle timer living in the renderer is requested to notify the idle state
@@ -694,6 +707,12 @@ TEST_P(ServiceWorkerActivationTest, LameDuckTime_SkipWaiting) {
   EXPECT_TRUE(*result);
   EXPECT_EQ(version_2.get(), reg->active_version());
   EXPECT_FALSE(IsLameDuckTimerRunning());
+
+  // Restore the TickClock to the default. This is required because the dtor
+  // of ServiceWorkerVersions can access the TickClock and it should outlive
+  // ServiceWorkerVersion.
+  version_1->SetTickClockForTesting(base::DefaultTickClock::GetInstance());
+  version_2->SetTickClockForTesting(base::DefaultTickClock::GetInstance());
 }
 
 // Test lame duck timer triggered by loss of controllee.
@@ -711,7 +730,7 @@ TEST_P(ServiceWorkerActivationTest, LameDuckTime_NoControllee) {
   // Remove the controllee. Since there is still an in-flight request,
   // activation should not happen. But the lame duck timer should start.
   EXPECT_FALSE(IsLameDuckTimerRunning());
-  version_1->RemoveControllee(controllee()->client_uuid());
+  RemoveControllee();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(version_1.get(), reg->active_version());
   EXPECT_TRUE(IsLameDuckTimerRunning());
@@ -721,12 +740,12 @@ TEST_P(ServiceWorkerActivationTest, LameDuckTime_NoControllee) {
   clock_1.Advance(kLittleBit);
 
   // Add a controllee again to reset the lame duck period.
-  version_1->AddControllee(controllee());
+  AddControllee();
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(IsLameDuckTimerRunning());
 
   // Remove the controllee.
-  version_1->RemoveControllee(controllee()->client_uuid());
+  RemoveControllee();
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(IsLameDuckTimerRunning());