ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

content/browser/service_worker/service_worker_version.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_VERSION_H_
 #define CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_VERSION_H_
 
 #include <stdint.h>
 
 #include <functional>
@@ skipping 47 matching lines @@
 
 // This class corresponds to a specific version of a ServiceWorker
 // script for a given pattern. When a script is upgraded, there may be
 // more than one ServiceWorkerVersion "running" at a time, but only
 // one of them is activated. This class connects the actual script with a
 // running worker.
 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,
     STARTING = EmbeddedWorkerInstance::STARTING,
     RUNNING = EmbeddedWorkerInstance::RUNNING,
     STOPPING = EmbeddedWorkerInstance::STOPPING,
   };
 
   // Current version status; some of the status (e.g. INSTALLED and ACTIVATED)
   // should be persisted unlike running status.
@@ skipping 146 matching lines @@
   // Connects to a specific mojo service exposed by the (running) service
   // worker. If a connection to a service for the same Interface already exists
   // this will return that existing connection. The |request_id| must be a value
   // previously returned by StartRequest. If the connection to the service
   // fails or closes before the request finished, the error callback associated
   // with |request_id| is called.
   // Only call GetMojoServiceForRequest once for a specific |request_id|.
   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);
 
-  // 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|.
+  // 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);
+
+  // 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
   // FinishRequest before passing the reply to the callback.
   template <typename ResponseMessage>
   void DispatchSimpleEvent(int request_id, const IPC::Message& message);
 
   // Adds and removes |provider_host| as a controllee of this ServiceWorker.
   // A potential controllee is a host having the version as its .installing
   // or .waiting version.
@@ skipping 90 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionBrowserTest,
                            TimeoutWorkerInEvent);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerStallInStoppingTest, DetachThenStart);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerStallInStoppingTest, DetachThenRestart);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest,
                            RegisterForeignFetchScopes);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest, RequestCustomizedTimeout);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest,
                            RequestCustomizedTimeoutKill);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest, MixedRequestTimeouts);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerURLRequestJobTest, EarlyResponse);
 
   class Metrics;
   class PingController;
 
   struct RequestInfo {
     RequestInfo(int id,
                 ServiceWorkerMetrics::EventType event_type,
                 const base::TimeTicks& expiration,
                 TimeoutBehavior timeout_behavior);
     ~RequestInfo();
@@ skipping 13 matching lines @@
 
     CallbackType callback;
     base::TimeTicks start_time;
     ServiceWorkerMetrics::EventType event_type;
     // Name of the mojo service this request is associated with. Used to call
     // the callback when a connection closes with outstanding requests.
     // Compared as pointer, so should only contain static strings. Typically
     // 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
   // arbitrary interfaces. The destructor is also responsible for calling the
   // error callbacks for any outstanding requests using this service.
   class CONTENT_EXPORT BaseMojoServiceWrapper {
    public:
     BaseMojoServiceWrapper(ServiceWorkerVersion* worker,
                            const char* service_name);
     virtual ~BaseMojoServiceWrapper();
@@ skipping 24 matching lines @@
     mojo::InterfacePtr<Interface> interface_;
     base::WeakPtrFactory<Interface> weak_ptr_factory_;
   };
 
   typedef ServiceWorkerVersion self;
   using ServiceWorkerClients = std::vector<ServiceWorkerClientInfo>;
   using RequestInfoPriorityQueue =
       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
   // the IPC message to listen for, while CallbackType should be a callback
   // with same arguments as the IPC message.
   // Additionally only calls the callback for messages with a specific request
   // id, which must be the first argument of the IPC message.
   template <typename ResponseMessage, typename CallbackType>
   class EventResponseHandler : public EmbeddedWorkerInstance::Listener {
    public:
@@ skipping 258 matching lines @@
     interface.set_connection_error_handler(
         base::Bind(&ServiceWorkerVersion::OnMojoConnectionError,
                    weak_factory_.GetWeakPtr(), Interface::Name_));
     service = new MojoServiceWrapper<Interface>(this, std::move(interface));
     mojo_services_.add(Interface::Name_, base::WrapUnique(service));
   }
   request->mojo_service = Interface::Name_;
   return service->GetWeakPtr();
 }
 
-template <typename ResponseMessage, typename ResponseCallbackType>
-void ServiceWorkerVersion::DispatchEvent(int request_id,
-                                         const IPC::Message& message,
-                                         const ResponseCallbackType& callback) {
-  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);
-  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));
-  }
-}
-
 template <typename ResponseMessage>
 void ServiceWorkerVersion::DispatchSimpleEvent(int request_id,
                                                const IPC::Message& message) {
-  DispatchEvent<ResponseMessage>(
-      request_id, message,
+  RegisterSimpleRequest<ResponseMessage>(request_id);
+  DispatchEvent({request_id}, message);
+}
+
+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));
 }
 
 template <typename ResponseMessage, typename CallbackType>
 bool ServiceWorkerVersion::EventResponseHandler<ResponseMessage, CallbackType>::
     OnMessageReceived(const IPC::Message& message) {
   if (message.type() != ResponseMessage::ID)
     return false;
   int received_request_id;
   bool result = base::PickleIterator(message).ReadInt(&received_request_id);
   if (!result || received_request_id != request_id_)
     return false;
 
   CallbackType protect(callback_);
   // Essentially same code as what IPC_MESSAGE_FORWARD expands to.
   void* param = nullptr;
   if (!ResponseMessage::Dispatch(&message, &callback_, this, param,
                                  &CallbackType::Run))
     message.set_dispatch_error();
 
   // At this point |this| can have been deleted, so don't do anything other
   // than returning.
 
   return true;
 }
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_VERSION_H_