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..c6a0d0cf7926af9d9e68e5f11dba71e90a8b3b47 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,39 @@ 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. If dispatching the event fails, all of the error
+  // callbacks associated with the |request_ids| are called.

[falken, 2016/06/09 19:28:48]

Let's mention these are the error callbacks from StartRequest. It's a bit
confusing there are callbacks passed to both StartRequest and
RegisterRequestCallback.

[shimazu, 2016/06/10 10:26:49]

Done.

I think it's a bit confusing, too. It looks better to integrate StartRequest
into DispatchEvent, but probably StartRequest needs to have the error callback
because BackgroundSync is using Mojo instead of using DispatchEvent.

+  // This method assumes you already registered callbacks to receive the IPCs.

[falken, 2016/06/09 19:28:48]

The reader might think "to receive the IPCs" means on the renderer-side, since
this function takes an IPC::Message.

[shimazu, 2016/06/10 10:26:49]

Done.

+  // Any messages sent back in response to this event will be passed on to the
+  // response callback which is registered beforehand.

[falken, 2016/06/09 19:28:48]

...and I think this is now somewhat redundant with the previous sentence.

Maybe it's clearer to say "Use RegisterRequestCallback or RegisterSimpleRequest
to register a callback to receive messages sent back in response to this event.
The callback must be registered before  DispatchEvent is called."

[shimazu, 2016/06/10 10:26:49]

Done.

+  // This must be called when the worker is running.
+  void DispatchEvent(const std::vector<int>& request_ids,
+                     const IPC::Message& message);
+
+  // This method registers a callback corresponding to |request_id| which
+  // receives and handles the message sent back from this SWVersion.

[falken, 2016/06/09 19:28:48]

It maybe a bit vague what "corresponding" means and what "the message" is. Also,
SWVersion is a browser-side concept. Maybe:

"Registers a callback to receive messages sent back from the service worker in
response to |request_id|."

[shimazu, 2016/06/10 10:26:49]

Done.

   // 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 one request_id.
   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);
+
+  // This method registers the simple callback corresponding to |request_id|.
+  // ResponseMessage must consist of just a request_id and
+  // a blink::WebServiceWorkerEventResult field. The result is converted to

[falken, 2016/06/09 19:28:48]

It's a bit hard to follow this: it's not clear that the third sentence is
describing what the "simple callback" from the first sentence does.

[shimazu, 2016/06/10 10:26:49]

Done.

+  // a ServiceWorkerStatusCode and passed to the error handler associated with
+  // the request_id which is registered at StartRequest. Additionally if you use
+  // this method, FinishRequest will be called before passing the reply to the
+  // callback.
+  // Callback registration should be done once for one request_id.
+  template <typename ResponseMessage>
+  void RegisterSimpleRequest(int request_id);
 
-  // For simple events where the full functionality of DispatchEvent is not
-  // needed, this method can be used instead. The ResponseMessage must consist
+  // This is a wrapper method integrating one RegisterSimpleRequest and one
+  // DispatchEvent. For simple events where the full functionality of

[falken, 2016/06/09 19:28:48]

The "integrating..." part is a bit awkward wording. I'd maybe say "A wrapper
method equivalent to one RegisterSimpleRequest and one DispatchEvent".

[shimazu, 2016/06/10 10:26:49]

Done.

+  // 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 +375,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 +409,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 = false;
   };
 
   // Base class to enable storing a list of mojo interface pointers for
@@ -432,6 +454,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 +734,30 @@ base::WeakPtr<Interface> ServiceWorkerVersion::GetMojoServiceForRequest(
   return service->GetWeakPtr();
 }
 
+template <typename ResponseMessage>
+void ServiceWorkerVersion::DispatchSimpleEvent(int request_id,
+                                               const IPC::Message& message) {
+  RegisterSimpleRequest<ResponseMessage>(request_id);
+  DispatchEvent({request_id}, message);
+}
+
 template <typename ResponseMessage, typename ResponseCallbackType>
-void ServiceWorkerVersion::DispatchEvent(int request_id,
-                                         const IPC::Message& message,
-                                         const ResponseCallbackType& callback) {
-  DCHECK_EQ(RUNNING, running_status());
+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) << "Request already dispatched an IPC event";
-
-  ServiceWorkerStatusCode status = embedded_worker_->SendMessage(message);
-  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));
-  }
+  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::DispatchSimpleEvent(int request_id,
-                                               const IPC::Message& message) {
-  DispatchEvent<ResponseMessage>(
-      request_id, message,
+void ServiceWorkerVersion::RegisterSimpleRequest(int request_id) {
+  RegisterRequestCallback<ResponseMessage>(
+      request_id,
       base::Bind(&ServiceWorkerVersion::OnSimpleEventResponse, this));
 }