Stop Service Workers that execute JavaScript for too long.

content/browser/service_worker/service_worker_version.cc

Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 352c33d7e7072c77468e88eb0f5054f286bcf823..7d16fc333689c4fca92a5464891f85ac19a7954e 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -84,6 +84,12 @@ const int64 kStopDoomedWorkerDelay = 5;  // 5 secs.
 // Default delay for scheduled update.
 const int kUpdateDelaySeconds = 1;
 
+// Delay between sending pings to the worker.
+const int kPingIntervalTime = 10;  // 10 secs.
+
+// Timeout for waiting for a response to a ping.
+const int kPingTimeoutTime = 30;  // 30 secs.

[kinuko, 2015/02/18 13:06:25]

nit: can you include these numbers in the CL description?

[falken, 2015/02/23 05:00:06]

Done.

+
 const char kClaimClientsStateErrorMesage[] =
     "Only the active worker can claim clients.";
 
@@ -284,6 +290,7 @@ ServiceWorkerVersion::ServiceWorkerVersion(
       status_(NEW),
       context_(context),
       script_cache_map_(this, context),
+      ping_timed_out_(false),
       is_doomed_(false),
       skip_waiting_(false),
       weak_factory_(this) {
@@ -792,11 +799,14 @@ void ServiceWorkerVersion::Doom() {
 
 void ServiceWorkerVersion::SetDevToolsAttached(bool attached) {
   embedded_worker()->set_devtools_attached(attached);
-  if (!attached && !stop_worker_timer_.IsRunning()) {
-    // If devtools is detached from this version and stop-worker-timer is not
-    // running, try scheduling stop-worker-timer now.
+  if (attached)
+    return;
+  // If devtools is detached try scheduling the timers for stopping the worker
+  // now.
+  if (!stop_worker_timer_.IsRunning())
     ScheduleStopWorker();
-  }
+  if (!ping_worker_timer_.IsRunning())
+    StartPingWorker();
 }
 
 void ServiceWorkerVersion::SetMainScriptHttpResponseInfo(
@@ -809,6 +819,11 @@ ServiceWorkerVersion::GetMainScriptHttpResponseInfo() {
   return main_script_http_info_.get();
 }
 
+void ServiceWorkerVersion::OnScriptLoaded() {
+  DCHECK_EQ(STARTING, running_status());
+  StartPingWorker();
+}
+
 void ServiceWorkerVersion::OnStarted() {
   DCHECK_EQ(RUNNING, running_status());
   DCHECK(cache_listener_.get());
@@ -828,6 +843,10 @@ void ServiceWorkerVersion::OnStopped(
   bool should_restart = !is_doomed() && !start_callbacks_.empty() &&
                         (old_status != EmbeddedWorkerInstance::STARTING);
 
+  ping_worker_timer_.Stop();
+  if (ping_timed_out_)
+    should_restart = false;
+
   // Fire all stop callbacks.
   RunCallbacks(this, &stop_callbacks_, SERVICE_WORKER_OK);
 
@@ -937,6 +956,7 @@ bool ServiceWorkerVersion::OnMessageReceived(const IPC::Message& message) {
                         OnSkipWaiting)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_ClaimClients,
                         OnClaimClients)
+    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_Pong, OnPongFromWorker)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -1327,6 +1347,12 @@ void ServiceWorkerVersion::OnClaimClients(int request_id) {
   registration->ClaimClients(callback);
 }
 
+void ServiceWorkerVersion::OnPongFromWorker() {
+  if (ping_timed_out_)
+    return;
+  SchedulePingWorker();
+}
+
 void ServiceWorkerVersion::DidClaimClients(
     int request_id, ServiceWorkerStatusCode status) {
   if (status == SERVICE_WORKER_ERROR_STATE) {
@@ -1365,6 +1391,44 @@ void ServiceWorkerVersion::DidGetClientInfo(
   callback->AddClientInfo(client_id, info);
 }
 
+void ServiceWorkerVersion::PingWorker() {
+  if (running_status() != STARTING && running_status() != RUNNING)
+    return;
+  ServiceWorkerStatusCode status =
+      embedded_worker_->SendMessage(ServiceWorkerMsg_Ping());
+  if (status != SERVICE_WORKER_OK) {
+    OnPingTimeout();

[kinuko, 2015/02/18 13:06:25]

Call this here feels a bit weird to me, especially if we start to show a notice
message in the method?

[falken, 2015/02/23 05:00:06]

Hmm, actually what do you think is best to do here? If we can't send Ping we
probably can't send Stop. Generally we may need StopWorkerIfIdle to handle
failure to send Stop and forcibly fail the callbacks so no one gets stuck
waiting for the worker.

I've added a TODO here and changed this to call StopWorkerIfIdle directly.

+    return;
+  }
+  ping_worker_timer_.Start(FROM_HERE,
+                           base::TimeDelta::FromSeconds(kPingTimeoutTime),
+                           base::Bind(&ServiceWorkerVersion::OnPingTimeout,
+                                      weak_factory_.GetWeakPtr()));
+}
+
+void ServiceWorkerVersion::StartPingWorker() {
+  ping_timed_out_ = false;
+  SchedulePingWorker();
+}
+
+void ServiceWorkerVersion::SchedulePingWorker() {
+  DCHECK(!ping_timed_out_);
+  ping_worker_timer_.Stop();
+  ping_worker_timer_.Start(FROM_HERE,
+                           base::TimeDelta::FromSeconds(kPingIntervalTime),
+                           base::Bind(&ServiceWorkerVersion::PingWorker,
+                                      weak_factory_.GetWeakPtr()));
+}
+
+void ServiceWorkerVersion::OnPingTimeout() {
+  if (running_status() != STARTING && running_status() != RUNNING)
+    return;
+  ping_timed_out_ = true;
+  // TODO(falken): Show a message to the developer that the SW was stopped due
+  // to timeout (crbug.com/457968).
+  StopWorkerIfIdle();
+}
+
 void ServiceWorkerVersion::ScheduleStopWorker() {
   if (running_status() != RUNNING)
     return;
@@ -1377,10 +1441,7 @@ void ServiceWorkerVersion::ScheduleStopWorker() {
 }
 
 void ServiceWorkerVersion::StopWorkerIfIdle() {
-  // Reschedule the stop the worker while there're inflight requests.
-  // (Note: we'll probably need to revisit this so that we can kill 'bad' SW.
-  // See https://github.com/slightlyoff/ServiceWorker/issues/527)
-  if (HasInflightRequests()) {
+  if (HasInflightRequests() && !ping_timed_out_) {
     ScheduleStopWorker();
     return;
   }