ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

content/browser/service_worker/service_worker_version.h

Index: content/browser/service_worker/service_worker_version.h
diff --git a/content/browser/service_worker/service_worker_version.h b/content/browser/service_worker/service_worker_version.h
index 9b88f791b286971ad5e1ad1853fa8260df2dfe62..f9e2763775d44fb88e324d68696f3cc4f89ebf86 100644
--- a/content/browser/service_worker/service_worker_version.h
+++ b/content/browser/service_worker/service_worker_version.h
@@ -65,7 +65,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
     : NON_EXPORTED_BASE(public base::RefCounted<ServiceWorkerVersion>),
       public EmbeddedWorkerInstance::Listener {
  public:
-  typedef base::Callback<void(ServiceWorkerStatusCode)> StatusCallback;
+  using StatusCallback = base::Callback<void(ServiceWorkerStatusCode)>;
 
   enum RunningStatus {
     STOPPED = EmbeddedWorkerInstance::STOPPED,
@@ -232,19 +232,42 @@ class CONTENT_EXPORT ServiceWorkerVersion
   template <typename Interface>
   base::WeakPtr<Interface> GetMojoServiceForRequest(int request_id);
 
-  // Dispatches an event. If dispatching the event fails, the error callback
-  // associated with the |request_id| is called. Any messages sent back in
-  // response to this event are passed on to the response |callback|.
+  // Dispatches an event. This method assumes you already registered callbacks
+  // to receive the IPCs and does not create EventResponseHandler. If
+  // dispatching the event fails, all of the error callbacks associated with the
+  // |request_id|s are called. Any messages sent back in  response to this event
+  // are passed on to the response |callback|.
+  // This must be called when the worker is running.
+  template <typename... RequestIDs>
+  ServiceWorkerStatusCode DispatchEvent(const IPC::Message& message,
+                                        int request_id,
+                                        const RequestIDs&... rest);

[falken, 2016/06/06 10:01:40]

Does RequestIDs need to be a template type? I'd rather keep this dump and simple
if possible.
Can you just have one-int and two-int overloaded functions? Or take in a vector
of ints?

[shimazu, 2016/06/07 02:27:40]

Yes, vector<int> and two-int arguments will also work, but I thought
variable-length arguments is better because it's more general. This can be used
for any number of responses. Moreover, practically we will use this for only two
or three replies, so I thought template is more handy than vector<int> for the
users of this function.

[falken, 2016/06/07 07:56:17]

I'd prefer either vector<int> or two-int over the template. It'll keep the code
and stacktraces simple. There's not many uses of typename... in our codebase.

[shimazu, 2016/06/09 05:44:21]

I see. I don't have so much preference for the template. As you said, vector<>
is simpler stacktrace and codes.
Done.

+  ServiceWorkerStatusCode DispatchEvent(const IPC::Message& message);
+
+  // This method registers a callback corresponding to |request_id|, but
+  // this doesn't dispatch a message.
   // ResponseMessage is the type of the IPC message that is used for the
   // response, and its first argument MUST be the request_id.
-  // This must be called when the worker is running.
+  // Callback registration should be done once for each request.
   template <typename ResponseMessage, typename ResponseCallbackType>
-  void DispatchEvent(int request_id,
-                     const IPC::Message& message,
-                     const ResponseCallbackType& callback);
+  void RegisterRequestCallback(int request_id,
+                               const ResponseCallbackType& callback);
 
-  // For simple events where the full functionality of DispatchEvent is not
-  // needed, this method can be used instead. The ResponseMessage must consist
+  // This method registers the simple callback corresponding to |request_id|,
+  // but this doesn't dispatch a message.  The ResponseMessage must consist
+  // of just a request_id and a blink::WebServiceWorkerEventResult field. The
+  // result is converted to a ServiceWorkerStatusCode and passed to the error
+  // handler associated with the request. Additionally this methods calls
+  // FinishRequest before passing the reply to the callback.
+  // This is internally used for DispatchSimpleEvent.
+  // Callback registration should be done once for each request.
+  template <typename ResponseMessage>
+  void RegisterSimpleRequest(int request_id);
+
+  // This is a wrapper method integrating one RegisterSimpleRequest and one
+  // DispatchEvent. For simple events where the full functionality of
+  // RegisterRequestCallback/DispatchEvent is not needed, this method can be
+  // used instead. The ResponseMessage must consist
   // of just a request_id and a blink::WebServiceWorkerEventResult field. The
   // result is converted to a ServiceWorkerStatusCode and passed to the error
   // handler associated with the request. Additionally this methods calls
@@ -355,6 +378,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest,
                            RequestCustomizedTimeoutKill);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest, MixedRequestTimeouts);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerURLRequestJobTest, EarlyResponse);
 
   class Metrics;
   class PingController;
@@ -388,6 +412,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
     // this would be Interface::Name_ for some mojo interface.
     const char* mojo_service = nullptr;
     std::unique_ptr<EmbeddedWorkerInstance::Listener> listener;
+    bool is_dispatched;

[falken, 2016/06/06 10:01:40]

Need to initialize this.

[shimazu, 2016/06/09 05:44:21]

Done.

   };
 
   // Base class to enable storing a list of mojo interface pointers for
@@ -432,6 +457,8 @@ class CONTENT_EXPORT ServiceWorkerVersion
       std::priority_queue<RequestInfo,
                           std::vector<RequestInfo>,
                           std::greater<RequestInfo>>;
+  using WebStatusCallback =
+      base::Callback<void(int, blink::WebServiceWorkerEventResult)>;
 
   // EmbeddedWorkerInstance Listener implementation which calls a callback
   // on receiving a particular IPC message. ResponseMessage is the type of
@@ -710,32 +737,50 @@ base::WeakPtr<Interface> ServiceWorkerVersion::GetMojoServiceForRequest(
   return service->GetWeakPtr();
 }
 
-template <typename ResponseMessage, typename ResponseCallbackType>
-void ServiceWorkerVersion::DispatchEvent(int request_id,
-                                         const IPC::Message& message,
-                                         const ResponseCallbackType& callback) {
+template <typename... RequestIDs>
+ServiceWorkerStatusCode ServiceWorkerVersion::DispatchEvent(
+    const IPC::Message& message,
+    int request_id,
+    const RequestIDs&... rest) {
   DCHECK_EQ(RUNNING, running_status());
   PendingRequest<StatusCallback>* request = custom_requests_.Lookup(request_id);
   DCHECK(request) << "Invalid request id";
-  DCHECK(!request->listener) << "Request already dispatched an IPC event";
-
-  ServiceWorkerStatusCode status = embedded_worker_->SendMessage(message);
+  DCHECK(!request->is_dispatched) << "Request already dispatched an IPC event";
+  const ServiceWorkerStatusCode status = DispatchEvent(message, rest...);
   if (status != SERVICE_WORKER_OK) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(request->callback, status));
     custom_requests_.Remove(request_id);
   } else {
-    request->listener.reset(
-        new EventResponseHandler<ResponseMessage, ResponseCallbackType>(
-            embedded_worker()->AsWeakPtr(), request_id, callback));
+    request->is_dispatched = true;
   }
+  return status;
 }
 
 template <typename ResponseMessage>
 void ServiceWorkerVersion::DispatchSimpleEvent(int request_id,
                                                const IPC::Message& message) {
-  DispatchEvent<ResponseMessage>(
-      request_id, message,
+  RegisterSimpleRequest<ResponseMessage>(request_id);
+  DispatchEvent(message, request_id);
+}
+
+template <typename ResponseMessage, typename ResponseCallbackType>
+void ServiceWorkerVersion::RegisterRequestCallback(
+    int request_id,
+    const ResponseCallbackType& callback) {
+  PendingRequest<StatusCallback>* request = custom_requests_.Lookup(request_id);
+  DCHECK(request) << "Invalid request id";
+  DCHECK(!request->listener) << "Callback was already registered";
+  DCHECK(!request->is_dispatched) << "Request already dispatched an IPC event";
+  request->listener.reset(
+      new EventResponseHandler<ResponseMessage, ResponseCallbackType>(
+          embedded_worker()->AsWeakPtr(), request_id, callback));
+}
+
+template <typename ResponseMessage>
+void ServiceWorkerVersion::RegisterSimpleRequest(int request_id) {
+  RegisterRequestCallback<ResponseMessage>(
+      request_id,
       base::Bind(&ServiceWorkerVersion::OnSimpleEventResponse, this));
 }