service worker: Remove "doom installing worker" mechanism.

The "doom installing worker" was introduced at a time when we didn't
have timeouts for starting a worker or for lifecycle events. The purpose
was to prevent the job queue from being stuck forever on a bad worker.
Now that we have these timeouts, we can remove the mechanism. This
also aligns with the spec, as seen in the passing WPT test.

A previous attempt was made at
https://chromium-review.googlesource.com/c/chromium/src/+/560633/.
That attempt added a timeout timer for the job queue, but was reverted
due to crashes with it. I considered adding a timeout timer here too,
but it seems to have little utility considering the job should only be
stalled by a worker stuck in starting or install. If there is another
failure, it's a bug and timing out might not actually help.

Bug: 723037, 999027
Change-Id: I0c6248db5a2e834f986218e61cec82e4b8e0f4a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1776156Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691996}

content/browser/service_worker/service_worker_job_coordinator.cc

@@ -15,14 +15,6 @@
 
 namespace content {
 
-namespace {
-
-bool IsRegisterJob(const ServiceWorkerRegisterJobBase& job) {
-  return job.GetType() == ServiceWorkerRegisterJobBase::REGISTRATION_JOB;
-}
-
-}  // namespace
-
 ServiceWorkerJobCoordinator::JobQueue::JobQueue() = default;
 
 ServiceWorkerJobCoordinator::JobQueue::JobQueue(JobQueue&&) = default;
@@ -39,7 +31,6 @@ ServiceWorkerRegisterJobBase* ServiceWorkerJobCoordinator::JobQueue::Push(
     StartOneJob();
   } else if (!job->Equals(jobs_.back().get())) {
     jobs_.push_back(std::move(job));
-    DoomInstallingWorkerIfNeeded();
   }
   // Note we are releasing 'job' here in case neither of the two if() statements
   // above were true.
@@ -56,25 +47,9 @@ void ServiceWorkerJobCoordinator::JobQueue::Pop(
     StartOneJob();
 }
 
-void ServiceWorkerJobCoordinator::JobQueue::DoomInstallingWorkerIfNeeded() {
-  DCHECK(!jobs_.empty());
-  if (!IsRegisterJob(*jobs_.front().get()))
-    return;
-  ServiceWorkerRegisterJob* job =
-      static_cast<ServiceWorkerRegisterJob*>(jobs_.front().get());
-  auto it = jobs_.begin();
-  for (++it; it != jobs_.end(); ++it) {
-    if (IsRegisterJob(**it)) {
-      job->DoomInstallingWorker();
-      return;
-    }
-  }
-}
-
 void ServiceWorkerJobCoordinator::JobQueue::StartOneJob() {
   DCHECK(!jobs_.empty());
   jobs_.front()->Start();
-  DoomInstallingWorkerIfNeeded();
 }
 
 void ServiceWorkerJobCoordinator::JobQueue::AbortAll() {

content/browser/service_worker/service_worker_job_coordinator.h

@@ -63,12 +63,6 @@ class CONTENT_EXPORT ServiceWorkerJobCoordinator {
     ServiceWorkerRegisterJobBase* Push(
         std::unique_ptr<ServiceWorkerRegisterJobBase> job);
 
-    // Dooms the installing worker of the running register/update job if a
-    // register/update job is scheduled to run after it. This corresponds to
-    // the "Terminate installing worker" steps at the beginning of the spec's
-    // [[Update]] and [[Install]] algorithms.
-    void DoomInstallingWorkerIfNeeded();
-
     // Starts the first job in the queue.
     void StartOneJob();
 

content/browser/service_worker/service_worker_job_unittest.cc

@@ -125,7 +125,8 @@ void RequestTermination(
 class ServiceWorkerJobTest : public testing::Test {
  public:
   ServiceWorkerJobTest()
-      : task_environment_(BrowserTaskEnvironment::IO_MAINLOOP) {}
+      : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME,
+                          BrowserTaskEnvironment::IO_MAINLOOP) {}
 
   void SetUp() override {
     helper_.reset(new EmbeddedWorkerTestHelper(base::FilePath()));
@@ -2015,4 +2016,85 @@ TEST_P(ServiceWorkerUpdateJobTest, ActivateCancelsOnShutdown) {
   runner->RunUntilIdle();
 }
 
+class WaitForeverInstallWorker : public FakeServiceWorker {
+ public:
+  WaitForeverInstallWorker(EmbeddedWorkerTestHelper* helper)
+      : FakeServiceWorker(helper) {}
+  ~WaitForeverInstallWorker() override = default;
+
+  void DispatchInstallEvent(
+      blink::mojom::ServiceWorker::DispatchInstallEventCallback callback)
+      override {
+    callback_ = std::move(callback);
+  }
+
+ private:
+  blink::mojom::ServiceWorker::DispatchInstallEventCallback callback_;
+};
+
+// Test that the job queue doesn't get stuck by bad workers.
+TEST_F(ServiceWorkerJobTest, TimeoutBadJobs) {
+  blink::mojom::ServiceWorkerRegistrationOptions options;
+  options.scope = GURL("https://www.example.com/");
+
+  // Make a job that gets stuck due to a worker stalled in starting.
+  base::RunLoop loop1;
+  scoped_refptr<ServiceWorkerRegistration> registration1;
+  helper_->AddPendingInstanceClient(
+      std::make_unique<DelayedFakeEmbeddedWorkerInstanceClient>(helper_.get()));
+  job_coordinator()->Register(
+      GURL("https://www.example.com/service_worker1.js"), options,
+      SaveRegistration(blink::ServiceWorkerStatusCode::kErrorTimeout,
+                       &registration1, loop1.QuitClosure()));
+
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+
+  // Make a job that gets stuck due to a worker that doesn't finish the install
+  // event. The callback is called with kOk, but the job will be stuck until
+  // the install event times out.
+  base::RunLoop loop2;
+  helper_->AddPendingServiceWorker(
+      std::make_unique<WaitForeverInstallWorker>(helper_.get()));
+  scoped_refptr<ServiceWorkerRegistration> registration2;
+  job_coordinator()->Register(
+      GURL("https://www.example.com/service_worker2.js"), options,
+      SaveRegistration(blink::ServiceWorkerStatusCode::kOk, &registration2,
+                       loop2.QuitClosure()));
+
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+
+  // Make a normal job.
+  base::RunLoop loop3;
+  scoped_refptr<ServiceWorkerRegistration> registration3;
+  job_coordinator()->Register(
+      GURL("https://www.example.com/service_worker3.js"), options,
+      SaveRegistration(blink::ServiceWorkerStatusCode::kOk, &registration3,
+                       loop3.QuitClosure()));
+
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+
+  // Timeout the first job.
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(2));
+  loop1.Run();
+
+  // Let the second job run until the install event is dispatched, then
+  // time out the event.
+  loop2.Run();
+  scoped_refptr<ServiceWorkerVersion> version =
+      registration2->installing_version();
+  ASSERT_TRUE(version);
+  EXPECT_EQ(ServiceWorkerVersion::INSTALLING, version->status());
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(5));
+  EXPECT_EQ(ServiceWorkerVersion::REDUNDANT, version->status());
+
+  // Let the third job finish successfully. It might have already been
+  // progressing so we don't know what state its worker is in, but it will
+  // eventually reach ACTIVATED.
+  loop3.Run();
+  version = registration3->GetNewestVersion();
+  ASSERT_TRUE(version);
+  TestServiceWorkerObserver observer(helper_->context_wrapper());
+  observer.RunUntilStatusChange(version.get(), ServiceWorkerVersion::ACTIVATED);
+}
+
 }  // namespace content

content/browser/service_worker/service_worker_register_job.cc

@@ -48,7 +48,6 @@ ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
       worker_script_type_(options.type),
       update_via_cache_(options.update_via_cache),
       phase_(INITIAL),
-      doom_installing_worker_(false),
       is_promise_resolved_(false),
       should_uninstall_on_failure_(false),
       force_bypass_cache_(false),
@@ -65,7 +64,6 @@ ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
       scope_(registration->scope()),
       update_via_cache_(registration->update_via_cache()),
       phase_(INITIAL),
-      doom_installing_worker_(false),
       is_promise_resolved_(false),
       should_uninstall_on_failure_(false),
       force_bypass_cache_(force_bypass_cache),
@@ -153,13 +151,6 @@ RegistrationJobType ServiceWorkerRegisterJob::GetType() const {
   return job_type_;
 }
 
-void ServiceWorkerRegisterJob::DoomInstallingWorker() {
-  doom_installing_worker_ = true;
-  if (phase_ == INSTALL)
-    Complete(blink::ServiceWorkerStatusCode::kErrorInstallWorkerFailed,
-             std::string());
-}
-
 ServiceWorkerRegisterJob::Internal::Internal() {}
 
 ServiceWorkerRegisterJob::Internal::~Internal() {}
@@ -562,12 +553,6 @@ void ServiceWorkerRegisterJob::InstallAndContinue() {
       ServiceWorkerMetrics::EventType::INSTALL,
       base::BindOnce(&ServiceWorkerRegisterJob::DispatchInstallEvent,
                      weak_factory_.GetWeakPtr()));
-
-  // A subsequent registration job may terminate our installing worker. It can
-  // only do so after we've started the worker and dispatched the install
-  // event, as those are atomic substeps in the [[Install]] algorithm.
-  if (doom_installing_worker_)
-    Complete(blink::ServiceWorkerStatusCode::kErrorInstallWorkerFailed);
 }
 
 void ServiceWorkerRegisterJob::DispatchInstallEvent(

content/browser/service_worker/service_worker_register_job.h

@@ -189,7 +189,6 @@ class ServiceWorkerRegisterJob : public ServiceWorkerRegisterJobBase {
   std::vector<RegistrationCallback> callbacks_;
   Phase phase_;
   Internal internal_;
-  bool doom_installing_worker_;
   bool is_promise_resolved_;
   bool should_uninstall_on_failure_;
   bool force_bypass_cache_;

third_party/blink/web_tests/external/wpt/service-workers/service-worker/register-wait-forever-in-install-worker.https-expected.txt

-This is a testharness.js-based test.
-FAIL register worker that calls waitUntil with a promise that never resolves in oninstall promise_test: Unhandled rejection with value: object "TypeError: Cannot read property 'scriptURL' of null"
-Harness: the test ran to completion.
-

third_party/blink/web_tests/http/tests/serviceworker/register-wait-forever-in-install-worker.html

-<!DOCTYPE html>
-<!-- This test cannot be upstreamed to WPT because it is maintained only to
-  preserve test coverage until such time as the more rigorous version available
-  in the Web Platform Tests project can be made to pass. See
-  https://crbug.com/723037 -->
-<title>Service Worker: Register wait-forever-in-install-worker</title>
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<script src="resources/test-helpers.js"></script>
-<script>
-
-promise_test(function(t) {
-    var bad_script = 'resources/wait-forever-in-install-worker.js';
-    var good_script = 'resources/empty-worker.js';
-    var scope = 'resources/wait-forever-in-install-worker';
-    return navigator.serviceWorker.register(bad_script, {scope: scope})
-      .then(function(registration) {
-          assert_equals(registration.installing.scriptURL,
-                        normalizeURL(bad_script));
-          return navigator.serviceWorker.register(good_script, {scope: scope});
-        })
-      .then(function(registration) {
-          assert_equals(registration.installing.scriptURL,
-                        normalizeURL(good_script));
-          return wait_for_state(t, registration.installing, 'activated');
-        })
-      .then(function() {
-          return service_worker_unregister_and_done(t, scope);
-        })
-  }, 'register worker that calls waitUntil with a promise that never ' +
-     'resolves in oninstall');
-
-</script>

third_party/blink/web_tests/http/tests/serviceworker/resources/wait-forever-in-install-worker.js

-self.addEventListener('install', function(event) {
-    event.waitUntil(new Promise(function() {}));
-  });