Service Worker: Add a timeout for inflight requests.

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 a1b225b36b96dd0313f51bddc91256d684cc213f..78e8447be423a64e343f46c47395a5c6f5d387b4 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -92,6 +92,19 @@ void RunIDMapCallbacks(IDMAP* callbacks, const Params&... params) {
   callbacks->Clear();
 }
 
+template <typename CallbackType, typename... Params>
+bool RunIDMapCallback(IDMap<CallbackType, IDMapOwnPointer>* callbacks,
+                      int request_id,
+                      const Params&... params) {
+  CallbackType* callback = callbacks->Lookup(request_id);
+  if (!callback)
+    return false;
+
+  callback->Run(params...);
+  callbacks->Remove(request_id);
+  return true;
+}
+
 void RunStartWorkerCallback(
     const StatusCallback& callback,
     scoped_refptr<ServiceWorkerRegistration> protect,
@@ -142,7 +155,7 @@ void RunErrorMessageCallback(
 void RunErrorCrossOriginConnectCallback(
     const ServiceWorkerVersion::CrossOriginConnectCallback& callback,
     ServiceWorkerStatusCode status) {
-  callback.Run(status, false);
+  callback.Run(status, false /* accept_connection */);
 }
 
 using WindowOpenedCallback = base::Callback<void(int, int)>;
@@ -304,6 +317,7 @@ void OnGetClientsFromUI(
 }  // namespace
 
 const int ServiceWorkerVersion::kStartWorkerTimeoutMinutes = 5;
+const int ServiceWorkerVersion::kRequestTimeoutMinutes = 5;

[kinuko, 2015/03/25 04:18:03]

I assume we can change this value and merge it if this change introduces
undesirable consequence...

[falken, 2015/03/25 06:10:00]

Yes that's reasonable. Or just skip pushing onto |requests_| completely in
AddRequest.

 
 ServiceWorkerVersion::ServiceWorkerVersion(
     ServiceWorkerRegistration* registration,
@@ -536,7 +550,7 @@ void ServiceWorkerVersion::DispatchFetchEvent(
 
   prepare_callback.Run();
 
-  int request_id = fetch_callbacks_.Add(new FetchCallback(fetch_callback));
+  int request_id = AddRequest(fetch_callback, &fetch_callbacks_, REQUEST_FETCH);
   ServiceWorkerStatusCode status = embedded_worker_->SendMessage(
       ServiceWorkerMsg_FetchEvent(request_id, request));
   if (status != SERVICE_WORKER_OK) {
@@ -566,7 +580,7 @@ void ServiceWorkerVersion::DispatchSyncEvent(const StatusCallback& callback) {
     return;
   }
 
-  int request_id = sync_callbacks_.Add(new StatusCallback(callback));
+  int request_id = AddRequest(callback, &sync_callbacks_, REQUEST_SYNC);
   ServiceWorkerStatusCode status = embedded_worker_->SendMessage(
       ServiceWorkerMsg_SyncEvent(request_id));
   if (status != SERVICE_WORKER_OK) {
@@ -591,8 +605,8 @@ void ServiceWorkerVersion::DispatchNotificationClickEvent(
     return;
   }
 
-  int request_id =
-      notification_click_callbacks_.Add(new StatusCallback(callback));
+  int request_id = AddRequest(callback, &notification_click_callbacks_,
+                              REQUEST_NOTIFICATION_CLICK);
   ServiceWorkerStatusCode status = embedded_worker_->SendMessage(
       ServiceWorkerMsg_NotificationClickEvent(request_id,
                                               notification_id,
@@ -616,7 +630,7 @@ void ServiceWorkerVersion::DispatchPushEvent(const StatusCallback& callback,
     return;
   }
 
-  int request_id = push_callbacks_.Add(new StatusCallback(callback));
+  int request_id = AddRequest(callback, &push_callbacks_, REQUEST_PUSH);
   ServiceWorkerStatusCode status = embedded_worker_->SendMessage(
       ServiceWorkerMsg_PushEvent(request_id, data));
   if (status != SERVICE_WORKER_OK) {
@@ -652,7 +666,8 @@ void ServiceWorkerVersion::DispatchGeofencingEvent(
     return;
   }
 
-  int request_id = geofencing_callbacks_.Add(new StatusCallback(callback));
+  int request_id =
+      AddRequest(callback, &geofencing_callbacks_, REQUEST_GEOFENCING);
   ServiceWorkerStatusCode status =
       embedded_worker_->SendMessage(ServiceWorkerMsg_GeofencingEvent(
           request_id, event_type, region_id, region));
@@ -683,8 +698,8 @@ void ServiceWorkerVersion::DispatchCrossOriginConnectEvent(
     return;
   }
 
-  int request_id = cross_origin_connect_callbacks_.Add(
-      new CrossOriginConnectCallback(callback));
+  int request_id = AddRequest(callback, &cross_origin_connect_callbacks_,
+                              REQUEST_CROSS_ORIGIN_CONNECT);
   ServiceWorkerStatusCode status = embedded_worker_->SendMessage(
       ServiceWorkerMsg_CrossOriginConnectEvent(request_id, client));
   if (status != SERVICE_WORKER_OK) {
@@ -780,9 +795,16 @@ void ServiceWorkerVersion::Doom() {
 void ServiceWorkerVersion::SetDevToolsAttached(bool attached) {
   embedded_worker()->set_devtools_attached(attached);
   if (attached) {
+    // TODO(falken): Canceling the timeouts when debugging could cause
+    // heisenbugs; we should instead run them as normal show an educational
+    // message in DevTools when they occur.

[kinuko, 2015/03/25 04:18:03]

Needs a bug?

[falken, 2015/03/25 06:10:00]

Done.

+
     // Don't record the startup time metric once DevTools is attached.
     ClearTick(&start_time_);
     skip_recording_startup_time_ = true;
+
+    // Cancel request timeouts.
+    SetAllRequestTimes(base::TimeTicks());
     return;
   }
   if (!start_callbacks_.empty()) {
@@ -792,6 +814,9 @@ void ServiceWorkerVersion::SetDevToolsAttached(bool attached) {
         << running_status();
     RestartTick(&start_time_);
   }
+
+  // Reactivate request timeouts.
+  SetAllRequestTimes(base::TimeTicks::Now());
 }
 
 void ServiceWorkerVersion::SetMainScriptHttpResponseInfo(
@@ -804,6 +829,13 @@ ServiceWorkerVersion::GetMainScriptHttpResponseInfo() {
   return main_script_http_info_.get();
 }
 
+ServiceWorkerVersion::RequestInfo::RequestInfo(int id, RequestType type)
+    : id(id), type(type), time(base::TimeTicks::Now()) {
+}
+
+ServiceWorkerVersion::RequestInfo::~RequestInfo() {
+}
+
 void ServiceWorkerVersion::OnScriptLoaded() {
   DCHECK_EQ(STARTING, running_status());
   // Activate ping/pong now that JavaScript execution will start.
@@ -966,7 +998,7 @@ void ServiceWorkerVersion::DispatchInstallEventAfterStartWorker(
   DCHECK_EQ(RUNNING, running_status())
       << "Worker stopped too soon after it was started.";
 
-  int request_id = install_callbacks_.Add(new StatusCallback(callback));
+  int request_id = AddRequest(callback, &install_callbacks_, REQUEST_INSTALL);
   ServiceWorkerStatusCode status = embedded_worker_->SendMessage(
       ServiceWorkerMsg_InstallEvent(request_id));
   if (status != SERVICE_WORKER_OK) {
@@ -980,7 +1012,7 @@ void ServiceWorkerVersion::DispatchActivateEventAfterStartWorker(
   DCHECK_EQ(RUNNING, running_status())
       << "Worker stopped too soon after it was started.";
 
-  int request_id = activate_callbacks_.Add(new StatusCallback(callback));
+  int request_id = AddRequest(callback, &activate_callbacks_, REQUEST_ACTIVATE);
   ServiceWorkerStatusCode status =
       embedded_worker_->SendMessage(ServiceWorkerMsg_ActivateEvent(request_id));
   if (status != SERVICE_WORKER_OK) {
@@ -1541,9 +1573,22 @@ void ServiceWorkerVersion::OnTimeoutTimer() {
     return;
   }
 
-  // This check occurs after the start_time_ timeout check, since in that case
-  // the start callbacks should fail with ERROR_TIMEOUT. In the other timeout
-  // checks, there's nothing more to do as the worker is already stopping.
+  // Requests have not finished within a certain period.
+  bool callback_timed_out = false;
+  while (!requests_.empty()) {
+    RequestInfo info = requests_.front();
+    if (GetTickDuration(info.time) <
+        base::TimeDelta::FromMinutes(kRequestTimeoutMinutes))
+      break;
+    if (OnRequestTimeout(info))
+      callback_timed_out = true;
+    requests_.pop();
+  }
+  if (callback_timed_out && running_status() != STOPPING)
+    embedded_worker_->Stop();
+
+  // For the timeouts below, there are no callbacks to timeout so there is
+  // nothing more to do if the worker is already stopping.
   if (running_status() == STOPPING)
     return;
 
@@ -1681,4 +1726,57 @@ void ServiceWorkerVersion::RemoveCallbackAndStopIfDoomed(
   }
 }
 
+template <typename CallbackType>
+int ServiceWorkerVersion::AddRequest(
+    const CallbackType& callback,
+    IDMap<CallbackType, IDMapOwnPointer>* callback_map,
+    RequestType request_type) {
+  int request_id = callback_map->Add(new CallbackType(callback));
+  requests_.push(RequestInfo(request_id, request_type));
+  return request_id;
+}
+
+bool ServiceWorkerVersion::OnRequestTimeout(const RequestInfo& info) {
+  switch (info.type) {
+    case REQUEST_ACTIVATE:
+      return RunIDMapCallback(&activate_callbacks_, info.id,
+                              SERVICE_WORKER_ERROR_TIMEOUT);
+    case REQUEST_INSTALL:
+      return RunIDMapCallback(&install_callbacks_, info.id,
+                              SERVICE_WORKER_ERROR_TIMEOUT);
+    case REQUEST_FETCH:
+      return RunIDMapCallback(
+          &fetch_callbacks_, info.id, SERVICE_WORKER_ERROR_TIMEOUT,
+          /* The other args are ignored for non-OK status. */
+          SERVICE_WORKER_FETCH_EVENT_RESULT_FALLBACK, ServiceWorkerResponse());
+    case REQUEST_SYNC:
+      return RunIDMapCallback(&sync_callbacks_, info.id,
+                              SERVICE_WORKER_ERROR_TIMEOUT);
+    case REQUEST_NOTIFICATION_CLICK:
+      return RunIDMapCallback(&notification_click_callbacks_, info.id,
+                              SERVICE_WORKER_ERROR_TIMEOUT);
+    case REQUEST_PUSH:
+      return RunIDMapCallback(&push_callbacks_, info.id,
+                              SERVICE_WORKER_ERROR_TIMEOUT);
+    case REQUEST_GEOFENCING:
+      return RunIDMapCallback(&geofencing_callbacks_, info.id,
+                              SERVICE_WORKER_ERROR_TIMEOUT);
+    case REQUEST_CROSS_ORIGIN_CONNECT:
+      return RunIDMapCallback(&cross_origin_connect_callbacks_, info.id,
+                              SERVICE_WORKER_ERROR_TIMEOUT,
+                              false /* accept_connection */);
+  }
+}
+
+void ServiceWorkerVersion::SetAllRequestTimes(const base::TimeTicks& ticks) {
+  std::queue<RequestInfo> new_callback_timeouts;
+  while (!requests_.empty()) {
+    RequestInfo info = requests_.front();
+    info.time = ticks;
+    new_callback_timeouts.push(info);
+    requests_.pop();
+  }
+  requests_.swap(new_callback_timeouts);
+}
+
 }  // namespace content