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 4da0c87085a4d50d6b5e0d8603ae7c82fa457652..12e2a072f19fa01aa084ea7f09f4af319e0394ce 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.
+
 const char kClaimClientsStateErrorMesage[] =
     "Only the active worker can claim clients.";
 
@@ -279,6 +285,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) {
@@ -787,11 +794,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())
+    SchedulePingWorker();
 }
 
 void ServiceWorkerVersion::SetMainScriptHttpResponseInfo(
@@ -804,6 +814,11 @@ ServiceWorkerVersion::GetMainScriptHttpResponseInfo() {
   return main_script_http_info_.get();
 }
 
+void ServiceWorkerVersion::OnScriptLoaded() {
+  DCHECK_EQ(STARTING, running_status());
+  SchedulePingWorker();

[michaeln, 2015/02/11 01:02:21]

Would it make sense to only turn the ping monitor on when HasInflightRequests()?
Might help avoid context switches and lighting up otherwise idle processes.

[falken, 2015/02/11 16:10:24]

That's a good thing to think about. In practice, ping will essentially only
happen in that case anyway since the stop_worker_timer_ will kill a worker that
doesn't have in-flight requests. Additionally, we want ping to kill a worker
that's taking too long to startup, which doesn't currently count as an in-flight
request.

So I'm thinking it should be OK as coded. It might be possible to fold the stop
timer and ping timer into one, or have more explicit coupling between the two,
but they are kind of two different forces: the stop timer tries to kill "idle"
workers and ping tries to kill "busy" ones.

+}
+
 void ServiceWorkerVersion::OnStarted() {
   DCHECK_EQ(RUNNING, running_status());
   DCHECK(cache_listener_.get());
@@ -823,6 +838,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);
 
@@ -932,6 +951,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;
@@ -1322,6 +1342,11 @@ void ServiceWorkerVersion::OnClaimClients(int request_id) {
   registration->ClaimClients(callback);
 }
 
+void ServiceWorkerVersion::OnPongFromWorker() {
+  ping_worker_timer_.Stop();
+  SchedulePingWorker();
+}
+
 void ServiceWorkerVersion::DidClaimClients(
     int request_id, ServiceWorkerStatusCode status) {
   if (status == SERVICE_WORKER_ERROR_STATE) {
@@ -1360,6 +1385,37 @@ 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();
+    return;
+  }
+  ping_worker_timer_.Start(FROM_HERE,
+                           base::TimeDelta::FromSeconds(kPingTimeoutTime),
+                           base::Bind(&ServiceWorkerVersion::OnPingTimeout,
+                                      weak_factory_.GetWeakPtr()));
+}
+
+void ServiceWorkerVersion::SchedulePingWorker() {
+  DCHECK(!ping_worker_timer_.IsRunning());
+  ping_timed_out_ = false;
+  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;
+  StopWorkerIfIdle();

[kinuko, 2015/02/10 09:15:52]

In this case we should probably show some error or warning to the developer.
Should we file a bug and add a comment for that?

[falken, 2015/02/11 16:10:24]

Yes I agree. I don't know a good place we'd show the error... maybe SW DevTools,
but if that's open we won't kill the worker.

+}
+
 void ServiceWorkerVersion::ScheduleStopWorker() {
   if (running_status() != RUNNING)
     return;
@@ -1372,10 +1428,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;
   }