Move notification click event dispatching out of ServiceWorkerVersion.

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 <map>
@@ skipping 34 matching lines @@
 }
 
 namespace content {
 
 class EmbeddedWorkerRegistry;
 class ServiceWorkerContextCore;
 class ServiceWorkerProviderHost;
 class ServiceWorkerRegistration;
 class ServiceWorkerURLRequestJob;
 struct NavigatorConnectClient;
-struct PlatformNotificationData;
 struct ServiceWorkerClientInfo;
 struct ServiceWorkerVersionInfo;
 struct TransferredMessagePort;
 
 // 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
@@ skipping 186 matching lines @@
 
   // Sends fetch event to the associated embedded worker and calls
   // |callback| with the response from the worker.
   //
   // This must be called when the status() is ACTIVATED. Calling this in other
   // statuses will result in an error SERVICE_WORKER_ERROR_FAILED.
   void DispatchFetchEvent(const ServiceWorkerFetchRequest& request,
                           const base::Closure& prepare_callback,
                           const FetchCallback& fetch_callback);
 
-  // Sends notificationclick event to the associated embedded worker and
-  // asynchronously calls |callback| when it errors out or it gets a response
-  // from the worker to notify completion.
-  //
-  // This must be called when the status() is ACTIVATED.
-  void DispatchNotificationClickEvent(
-      const StatusCallback& callback,
-      int64_t persistent_notification_id,
-      const PlatformNotificationData& notification_data,
-      int action_index);
-
   // Sends push event to the associated embedded worker and asynchronously calls
   // |callback| when it errors out or it gets a response from the worker to
   // notify completion.
   //
   // This must be called when the status() is ACTIVATED.
   void DispatchPushEvent(const StatusCallback& callback,
                          const std::string& data);
 
   // Sends a cross origin message event to the associated embedded worker and
   // asynchronously calls |callback| when the message was sent (or failed to
@@ skipping 113 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerWaitForeverInFetchTest,
                            MixedRequestTimeouts);
 
   class Metrics;
   class PingController;
 
   enum RequestType {
     REQUEST_ACTIVATE,
     REQUEST_INSTALL,
     REQUEST_FETCH,
-    REQUEST_NOTIFICATION_CLICK,
     REQUEST_PUSH,
     REQUEST_CUSTOM,
     NUM_REQUEST_TYPES
   };
 
   struct RequestInfo {
     RequestInfo(int id,
                 RequestType type,
                 ServiceWorkerMetrics::EventType event_type,
                 const base::TimeTicks& expiration,
@@ skipping 141 matching lines @@
   void OnGetClients(int request_id,
                     const ServiceWorkerClientQueryOptions& options);
 
   void OnActivateEventFinished(int request_id,
                                blink::WebServiceWorkerEventResult result);
   void OnInstallEventFinished(int request_id,
                               blink::WebServiceWorkerEventResult result);
   void OnFetchEventFinished(int request_id,
                             ServiceWorkerFetchEventResult result,
                             const ServiceWorkerResponse& response);
-  void OnNotificationClickEventFinished(int request_id);
   void OnPushEventFinished(int request_id,
                            blink::WebServiceWorkerEventResult result);
   void OnOpenWindow(int request_id, GURL url);
   void OnOpenWindowFinished(int request_id,
                             ServiceWorkerStatusCode status,
                             const std::string& client_uuid,
                             const ServiceWorkerClientInfo& client_info);
 
   void OnSetCachedMetadata(const GURL& url, const std::vector<char>& data);
   void OnSetCachedMetadataFinished(int64_t callback_id, int result);
@@ skipping 108 matching lines @@
   scoped_ptr<EmbeddedWorkerInstance> embedded_worker_;
   std::vector<StatusCallback> start_callbacks_;
   std::vector<StatusCallback> stop_callbacks_;
   std::vector<base::Closure> status_change_callbacks_;
 
   // Message callbacks. (Update HasInflightRequests() too when you update this
   // list.)
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> activate_requests_;
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> install_requests_;
   IDMap<PendingRequest<FetchCallback>, IDMapOwnPointer> fetch_requests_;
-  IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer>
-      notification_click_requests_;
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> push_requests_;
   IDMap<PendingRequest<StatusCallback>, IDMapOwnPointer> custom_requests_;
 
   // Stores all open connections to mojo services. Maps the service name to
   // the actual interface pointer. When a connection is closed it is removed
   // from this map.
   // mojo_services_[Interface::Name_] is assumed to always contain a
   // MojoServiceWrapper<Interface> instance.
   base::ScopedPtrHashMap<const char*, scoped_ptr<BaseMojoServiceWrapper>>
       mojo_services_;
@@ skipping 102 matching lines @@
 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_);

[johnme, 2016/01/18 17:57:05]

Why are you protecting the callback? Is that because you expect the callback to
have a scoped_refptr<ServiceWorkerVersion> to this?

If so, that'll break if the callback instead has a bound scoped_ptr to some
other class that holds a scoped_refptr<ServiceWorkerVersion>. In that case, once
the callback is run, the scoped_ptr's ownership will be passed to the bound
function, and subsequently destroyed unless the bound function does something
with it. Hence ServiceWorkerVersion's refcount will be decremented, and possibly
destroyed. Such callbacks can only be called once.

Instead, it seems safer to protect whatever you want to protect directly:

  ...

  {
    scoped_refptr<Something> protect(something);

    // 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;
}

[Marijn Kruisselbrink, 2016/01/19 23:50:17]

I am protecting the callback because that's what I need to keep alive till the
end of its Run() method. Any parameters bound to the callback and passed by
reference to the actual method being called are references to data owned by the
Callback object. Without this protect(), FinishRequest called from within the
callback would delete the EventResponseHandler, which held the last reference to
the callback, and any further use of those parameters from within the callback
would then be undefined behavior.

I don't believe there is anything else that needs protecting here. By the time
the callback returns |this| (the EventResponseHandler) can have been deleted,
and so this method doesn't do anything else afterwards anyway. And if any code
higher up in the stack needs something to still be alive after OnMessageReceived
returns there is nothing this method can do to protect that anyway (and this
isn't a change compared to the previous code either; we can protect objects to
stay alive for the duration of the callback, not after we return).

[johnme, 2016/01/20 17:07:10]

Ok, sounds reasonable. Who owns the |message|? Is is possible that could be
deleted?

   // 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_