service worker: Revert the self-disabling mechanism

Beta channel shows 0.1% of users have a disabled service worker, which still
seems too aggressive. This feature isn't ready for Stable. I intend to merge
this patch to M50.

It's now looking like a simple timeout is not a good heuristic for a broken
service worker, since even main frame navigation without SW can take over
10 seconds to begin a URLRequestJob (crbug.com/448722).

The mechanism to force creating a renderer process remains, to help service
worker escape stuck renderer processes.

BUG=559001
TBR=jwd

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

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

12 files changed, 48 insertions, 125 deletions

diff --git a/content/browser/notifications/notification_event_dispatcher_impl.cc b/content/browser/notifications/notification_event_dispatcher_impl.cc
index e19a1f8..032d503 100644
--- a/content/browser/notifications/notification_event_dispatcher_impl.cc
+++ b/content/browser/notifications/notification_event_dispatcher_impl.cc
@@ -78,7 +78,6 @@ void ServiceWorkerNotificationEventFinished(
     case SERVICE_WORKER_ERROR_DISK_CACHE:
     case SERVICE_WORKER_ERROR_REDUNDANT:
     case SERVICE_WORKER_ERROR_DISALLOWED:
-    case SERVICE_WORKER_ERROR_DISABLED_WORKER:
     case SERVICE_WORKER_ERROR_MAX_VALUE:
       status = PERSISTENT_NOTIFICATION_STATUS_SERVICE_WORKER_ERROR;
       break;
@@ -135,7 +134,6 @@ void DispatchNotificationEventOnRegistration(
     case SERVICE_WORKER_ERROR_DISK_CACHE:
     case SERVICE_WORKER_ERROR_REDUNDANT:
     case SERVICE_WORKER_ERROR_DISALLOWED:
-    case SERVICE_WORKER_ERROR_DISABLED_WORKER:
     case SERVICE_WORKER_ERROR_MAX_VALUE:
       status = PERSISTENT_NOTIFICATION_STATUS_SERVICE_WORKER_ERROR;
       break;
diff --git a/content/browser/push_messaging/push_messaging_message_filter.cc b/content/browser/push_messaging/push_messaging_message_filter.cc
index ef75724..14ee2cd 100644
--- a/content/browser/push_messaging/push_messaging_message_filter.cc
+++ b/content/browser/push_messaging/push_messaging_message_filter.cc
@@ -652,7 +652,6 @@ void PushMessagingMessageFilter::UnsubscribeHavingGottenSenderId(
     case SERVICE_WORKER_ERROR_DISK_CACHE:
     case SERVICE_WORKER_ERROR_REDUNDANT:
     case SERVICE_WORKER_ERROR_DISALLOWED:
-    case SERVICE_WORKER_ERROR_DISABLED_WORKER:
     case SERVICE_WORKER_ERROR_MAX_VALUE:
       NOTREACHED() << "Got unexpected error code: " << service_worker_status
                    << " " << ServiceWorkerStatusToString(service_worker_status);
@@ -849,7 +848,6 @@ void PushMessagingMessageFilter::DidGetSubscription(
     case SERVICE_WORKER_ERROR_DISK_CACHE:
     case SERVICE_WORKER_ERROR_REDUNDANT:
     case SERVICE_WORKER_ERROR_DISALLOWED:
-    case SERVICE_WORKER_ERROR_DISABLED_WORKER:
     case SERVICE_WORKER_ERROR_MAX_VALUE: {
       NOTREACHED() << "Got unexpected error code: " << service_worker_status
                    << " " << ServiceWorkerStatusToString(service_worker_status);
diff --git a/content/browser/push_messaging/push_messaging_router.cc b/content/browser/push_messaging/push_messaging_router.cc
index dce24a9..8e93fe7 100644
--- a/content/browser/push_messaging/push_messaging_router.cc
+++ b/content/browser/push_messaging/push_messaging_router.cc
@@ -141,7 +141,6 @@ void PushMessagingRouter::DeliverMessageEnd(
     case SERVICE_WORKER_ERROR_DISK_CACHE:
     case SERVICE_WORKER_ERROR_REDUNDANT:
     case SERVICE_WORKER_ERROR_DISALLOWED:
-    case SERVICE_WORKER_ERROR_DISABLED_WORKER:
       delivery_status = PUSH_DELIVERY_STATUS_SERVICE_WORKER_ERROR;
       break;
     case SERVICE_WORKER_ERROR_EXISTS:
diff --git a/content/browser/service_worker/service_worker_context_core.cc b/content/browser/service_worker/service_worker_context_core.cc
index 6ef5ee3..44e1a77 100644
--- a/content/browser/service_worker/service_worker_context_core.cc
+++ b/content/browser/service_worker/service_worker_context_core.cc
@@ -703,16 +703,9 @@ void ServiceWorkerContextCore::CheckHasServiceWorker(
 void ServiceWorkerContextCore::UpdateVersionFailureCount(
     int64_t version_id,
     ServiceWorkerStatusCode status) {
-  if (failure_counts_expiration_time_.is_null()) {
-    failure_counts_expiration_time_ =
-        base::Time::Now() + base::TimeDelta::FromHours(24);
-  }
-
   // Don't count these, they aren't start worker failures.
-  if (status == SERVICE_WORKER_ERROR_DISALLOWED ||
-      status == SERVICE_WORKER_ERROR_DISABLED_WORKER) {
+  if (status == SERVICE_WORKER_ERROR_DISALLOWED)
     return;
-  }
 
   auto it = failure_counts_.find(version_id);
   if (status == SERVICE_WORKER_OK) {
@@ -731,15 +724,6 @@ void ServiceWorkerContextCore::UpdateVersionFailureCount(
 }
 
 int ServiceWorkerContextCore::GetVersionFailureCount(int64_t version_id) {
-  // Periodically clear failure counts to give disabled versions a chance to
-  // start.
-  if (base::Time::Now() > failure_counts_expiration_time_) {
-    failure_counts_.clear();
-    failure_counts_expiration_time_ =
-        base::Time::Now() + base::TimeDelta::FromHours(24);
-    return 0;
-  }
-
   auto it = failure_counts_.find(version_id);
   if (it == failure_counts_.end())
     return 0;
diff --git a/content/browser/service_worker/service_worker_context_core.h b/content/browser/service_worker/service_worker_context_core.h
index 0c7a943..0defe34 100644
--- a/content/browser/service_worker/service_worker_context_core.h
+++ b/content/browser/service_worker/service_worker_context_core.h
@@ -288,9 +288,6 @@ class CONTENT_EXPORT ServiceWorkerContextCore
   }
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerFailToStartTest,
-                           FailingWorkerIsDisabled);
-
   typedef std::map<int64_t, ServiceWorkerRegistration*> RegistrationsMap;
   typedef std::map<int64_t, ServiceWorkerVersion*> VersionMap;
 
@@ -342,7 +339,6 @@ class CONTENT_EXPORT ServiceWorkerContextCore
   std::map<int64_t, ServiceWorkerVersion*> live_versions_;
   std::map<int64_t, scoped_refptr<ServiceWorkerVersion>> protected_versions_;
   std::map<int64_t /* version_id */, int /* count */> failure_counts_;
-  base::Time failure_counts_expiration_time_;
 
   // PlzNavigate
   // Map of ServiceWorkerNavigationHandleCores used for navigation requests.
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 8a28576..80d4343 100644
--- a/content/browser/service_worker/service_worker_controllee_request_handler.cc
+++ b/content/browser/service_worker/service_worker_controllee_request_handler.cc
@@ -245,25 +245,6 @@ ServiceWorkerControlleeRequestHandler::DidLookupRegistrationForMainResource(
     return;
   }
 
-  // Ignore a SW that failed too much as a safety measure.
-  // ServiceWorkerVersion::StartWorker would call back with failure
-  // automatically but we want the clear trace event here and avoid setting
-  // .controller just to reset it.
-  if (active_version.get() && active_version->IsDisabled()) {
-    job_->FallbackToNetwork();
-    TRACE_EVENT_ASYNC_END2(
-        "ServiceWorker",
-        "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
-        job_.get(), "Status", status, "Info",
-        "The SW was skipped because its start failure count is too high");
-
-    // Show a message in DevTools for developers.
-    active_version->ReportError(SERVICE_WORKER_ERROR_DISABLED_WORKER,
-                                "The service worker is disabled because its "
-                                "start failure count is too high.");
-    return;
-  }
-
   // A registration exists, so associate it. Note that the controller is only
   // set if there's an active version. If there's no active version, we should
   // still associate so the provider host can use .ready.
diff --git a/content/browser/service_worker/service_worker_registration_status.cc b/content/browser/service_worker/service_worker_registration_status.cc
index 433395e..46ff019 100644
--- a/content/browser/service_worker/service_worker_registration_status.cc
+++ b/content/browser/service_worker/service_worker_registration_status.cc
@@ -64,7 +64,6 @@ void GetServiceWorkerRegistrationStatusResponse(
     case SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED:
     case SERVICE_WORKER_ERROR_STATE:
     case SERVICE_WORKER_ERROR_DISK_CACHE:
-    case SERVICE_WORKER_ERROR_DISABLED_WORKER:
     case SERVICE_WORKER_ERROR_MAX_VALUE:
       // Unexpected, or should have bailed out before calling this, or we don't
       // have a corresponding blink error code yet.
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index d497cee..8302403 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -54,9 +54,6 @@ using StatusCallback = ServiceWorkerVersion::StatusCallback;
 
 namespace {
 
-// The number of start failures to allow before disabling the worker.
-const int kDisableWorkerFailureCountThreshold = 3;
-
 // Time to wait until stopping an idle worker.
 const int kIdleWorkerTimeoutSeconds = 30;
 
@@ -410,17 +407,6 @@ void ServiceWorkerVersion::StartWorker(ServiceWorkerMetrics::EventType purpose,
     RunSoon(base::Bind(callback, SERVICE_WORKER_ERROR_REDUNDANT));
     return;
   }
-  if (IsDisabled()) {
-    RecordStartWorkerResult(purpose, status_, kInvalidTraceId,
-                            SERVICE_WORKER_ERROR_DISABLED_WORKER);
-    RunSoon(base::Bind(callback, SERVICE_WORKER_ERROR_DISABLED_WORKER));
-
-    // Show a message in DevTools for developers.
-    ReportError(SERVICE_WORKER_ERROR_DISABLED_WORKER,
-                "The service worker is disabled because its start failure "
-                "count is too high.");
-    return;
-  }
 
   // Check that the worker is allowed to start on the given scope. Since this
   // worker might not be used for a specific frame/process, use -1.
@@ -755,11 +741,6 @@ void ServiceWorkerVersion::SimulatePingTimeoutForTesting() {
   ping_controller_->SimulateTimeoutForTesting();
 }
 
-bool ServiceWorkerVersion::IsDisabled() const {
-  return context_->GetVersionFailureCount(version_id_) >=
-         kDisableWorkerFailureCountThreshold;
-}
-
 const net::HttpResponseInfo*
 ServiceWorkerVersion::GetMainScriptHttpResponseInfo() {
   return main_script_http_info_.get();
diff --git a/content/browser/service_worker/service_worker_version.h b/content/browser/service_worker/service_worker_version.h
index 25b847e..2359edc 100644
--- a/content/browser/service_worker/service_worker_version.h
+++ b/content/browser/service_worker/service_worker_version.h
@@ -335,8 +335,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
   // Simulate ping timeout. Should be used for tests-only.
   void SimulatePingTimeoutForTesting();
 
-  bool IsDisabled() const;
-
  private:
   friend class base::RefCounted<ServiceWorkerVersion>;
   friend class ServiceWorkerMetrics;
diff --git a/content/browser/service_worker/service_worker_version_unittest.cc b/content/browser/service_worker/service_worker_version_unittest.cc
index 06bb32b..88660df 100644
--- a/content/browser/service_worker/service_worker_version_unittest.cc
+++ b/content/browser/service_worker/service_worker_version_unittest.cc
@@ -1279,71 +1279,62 @@ TEST_F(ServiceWorkerVersionTest, DispatchEvent) {
   EXPECT_TRUE(version_->FinishRequest(request_id, true));
 }
 
-TEST_F(ServiceWorkerFailToStartTest, FailingWorkerIsDisabled) {
+TEST_F(ServiceWorkerFailToStartTest, FailingWorkerUsesNewRendererProcess) {
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
 
   helper_->SimulateAddProcessToPattern(pattern_,
                                        helper_->new_render_process_id());
-
-  // (1) Fail twice and then succeed. The failure count should be reset.
   ServiceWorkerContextCore* context = helper_->context();
   int64_t id = version_->version_id();
-  set_start_mode(MessageReceiverDisallowStart::StartMode::FAIL);
-  // Fail once.
-  version_->StartWorker(ServiceWorkerMetrics::EventType::UNKNOWN,
-                        CreateReceiverOnCurrentThread(&status));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1, context->GetVersionFailureCount(id));
-  // Fail again.
-  version_->StartWorker(ServiceWorkerMetrics::EventType::UNKNOWN,
-                        CreateReceiverOnCurrentThread(&status));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(2, context->GetVersionFailureCount(id));
-  // Succeed. It should choose the "new process".
+
+  // Start once. It should choose the "existing process".
   set_start_mode(MessageReceiverDisallowStart::StartMode::SUCCEED);
   version_->StartWorker(ServiceWorkerMetrics::EventType::UNKNOWN,
                         CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(helper_->new_render_process_id(),
+  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  EXPECT_EQ(helper_->mock_render_process_id(),
             version_->embedded_worker()->process_id());
-  EXPECT_EQ(0, context->GetVersionFailureCount(id));
   version_->StopWorker(CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
 
-  // (2) Fail three times. The worker should be disabled.
-  set_start_mode(MessageReceiverDisallowStart::StartMode::FAIL);
   // Fail once.
+  set_start_mode(MessageReceiverDisallowStart::StartMode::FAIL);
   version_->StartWorker(ServiceWorkerMetrics::EventType::UNKNOWN,
                         CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(SERVICE_WORKER_ERROR_START_WORKER_FAILED, status);
   EXPECT_EQ(1, context->GetVersionFailureCount(id));
+
   // Fail again.
   version_->StartWorker(ServiceWorkerMetrics::EventType::UNKNOWN,
                         CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(SERVICE_WORKER_ERROR_START_WORKER_FAILED, status);
   EXPECT_EQ(2, context->GetVersionFailureCount(id));
-  // Fail again.
+
+  // Succeed. It should choose the "new process".
+  set_start_mode(MessageReceiverDisallowStart::StartMode::SUCCEED);
   version_->StartWorker(ServiceWorkerMetrics::EventType::UNKNOWN,
                         CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(3, context->GetVersionFailureCount(id));
+  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  EXPECT_EQ(helper_->new_render_process_id(),
+            version_->embedded_worker()->process_id());
+  EXPECT_EQ(0, context->GetVersionFailureCount(id));
+  version_->StopWorker(CreateReceiverOnCurrentThread(&status));
+  base::RunLoop().RunUntilIdle();
 
-  // The worker should be disabled.
-  EXPECT_TRUE(version_->IsDisabled());
-  set_start_mode(MessageReceiverDisallowStart::StartMode::SUCCEED);
+  // Start again. It should choose the "existing process" again as we no longer
+  // force creation of a new process.
   version_->StartWorker(ServiceWorkerMetrics::EventType::UNKNOWN,
                         CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(SERVICE_WORKER_ERROR_DISABLED_WORKER, status);
-  EXPECT_EQ(ServiceWorkerVersion::STOPPED, version_->running_status());
-
-  // (3) The worker is re-enabled when the failure counts expire.
-  EXPECT_TRUE(version_->IsDisabled());
-  base::Time yesterday = GetYesterday();
-  context->failure_counts_expiration_time_ = yesterday;
-  EXPECT_EQ(0, context->GetVersionFailureCount(id));
-  EXPECT_LT(yesterday, context->failure_counts_expiration_time_);
-  EXPECT_FALSE(version_->IsDisabled());
+  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  EXPECT_EQ(helper_->mock_render_process_id(),
+            version_->embedded_worker()->process_id());
+  version_->StopWorker(CreateReceiverOnCurrentThread(&status));
+  base::RunLoop().RunUntilIdle();
 }
 
 TEST_F(ServiceWorkerVersionTest, DispatchConcurrentEvent) {
diff --git a/content/common/service_worker/service_worker_status_code.cc b/content/common/service_worker/service_worker_status_code.cc
index c2a6f27..726f9bb 100644
--- a/content/common/service_worker/service_worker_status_code.cc
+++ b/content/common/service_worker/service_worker_status_code.cc
@@ -49,8 +49,6 @@ const char* ServiceWorkerStatusToString(ServiceWorkerStatusCode status) {
       return "Redundant worker";
     case SERVICE_WORKER_ERROR_DISALLOWED:
       return "Worker disallowed";
-    case SERVICE_WORKER_ERROR_DISABLED_WORKER:
-      return "Worker disabled";
     case SERVICE_WORKER_ERROR_MAX_VALUE:
       NOTREACHED();
   }
diff --git a/content/common/service_worker/service_worker_status_code.h b/content/common/service_worker/service_worker_status_code.h
index 9583315..24be816 100644
--- a/content/common/service_worker/service_worker_status_code.h
+++ b/content/common/service_worker/service_worker_status_code.h
@@ -13,71 +13,71 @@ namespace content {
 // This enum is used in UMA histograms. Append-only.
 enum ServiceWorkerStatusCode {
   // Operation succeeded.
-  SERVICE_WORKER_OK,
+  SERVICE_WORKER_OK = 0,
 
   // Generic operation error (more specific error code should be used in
   // general).
-  SERVICE_WORKER_ERROR_FAILED,
+  SERVICE_WORKER_ERROR_FAILED = 1,
 
   // Operation was aborted (e.g. due to context or child process shutdown).
-  SERVICE_WORKER_ERROR_ABORT,
+  SERVICE_WORKER_ERROR_ABORT = 2,
 
   // Starting a new service worker script context failed.
-  SERVICE_WORKER_ERROR_START_WORKER_FAILED,
+  SERVICE_WORKER_ERROR_START_WORKER_FAILED = 3,
 
   // Could not find a renderer process to run a service worker.
-  SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND,
+  SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND = 4,
 
   // Generic error code to indicate the specified item is not found.
-  SERVICE_WORKER_ERROR_NOT_FOUND,
+  SERVICE_WORKER_ERROR_NOT_FOUND = 5,
 
   // Generic error code to indicate the specified item already exists.
-  SERVICE_WORKER_ERROR_EXISTS,
+  SERVICE_WORKER_ERROR_EXISTS = 6,
 
   // Install event handling failed.
-  SERVICE_WORKER_ERROR_INSTALL_WORKER_FAILED,
+  SERVICE_WORKER_ERROR_INSTALL_WORKER_FAILED = 7,
 
   // Activate event handling failed.
-  SERVICE_WORKER_ERROR_ACTIVATE_WORKER_FAILED,
+  SERVICE_WORKER_ERROR_ACTIVATE_WORKER_FAILED = 8,
 
   // Sending an IPC to the worker failed (often due to child process is
   // terminated).
-  SERVICE_WORKER_ERROR_IPC_FAILED,
+  SERVICE_WORKER_ERROR_IPC_FAILED = 9,
 
   // Operation is failed by network issue.
-  SERVICE_WORKER_ERROR_NETWORK,
+  SERVICE_WORKER_ERROR_NETWORK = 10,
 
   // Operation is failed by security issue.
-  SERVICE_WORKER_ERROR_SECURITY,
+  SERVICE_WORKER_ERROR_SECURITY = 11,
 
   // Event handling failed (event.waitUntil Promise rejected).
-  SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
+  SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED = 12,
 
   // An error triggered by invalid worker state.
-  SERVICE_WORKER_ERROR_STATE,
+  SERVICE_WORKER_ERROR_STATE = 13,
 
   // The Service Worker took too long to finish a task.
-  SERVICE_WORKER_ERROR_TIMEOUT,
+  SERVICE_WORKER_ERROR_TIMEOUT = 14,
 
   // An error occurred during initial script evaluation.
-  SERVICE_WORKER_ERROR_SCRIPT_EVALUATE_FAILED,
+  SERVICE_WORKER_ERROR_SCRIPT_EVALUATE_FAILED = 15,
 
   // Generic error to indicate failure to read/write the disk cache.
-  SERVICE_WORKER_ERROR_DISK_CACHE,
+  SERVICE_WORKER_ERROR_DISK_CACHE = 16,
 
   // The worker is in REDUNDANT state.
-  SERVICE_WORKER_ERROR_REDUNDANT,
+  SERVICE_WORKER_ERROR_REDUNDANT = 17,
 
   // The worker was disallowed (by ContentClient: e.g., due to
   // browser settings).
-  SERVICE_WORKER_ERROR_DISALLOWED,
+  SERVICE_WORKER_ERROR_DISALLOWED = 18,
 
-  // The worker is disabled because it failed to start too many times.
-  SERVICE_WORKER_ERROR_DISABLED_WORKER,
+  // Obsolete.
+  // SERVICE_WORKER_ERROR_DISABLED_WORKER = 19,
 
   // Add new status codes here.
 
-  SERVICE_WORKER_ERROR_MAX_VALUE
+  SERVICE_WORKER_ERROR_MAX_VALUE = 20
 };
 
 CONTENT_EXPORT const char* ServiceWorkerStatusToString(