Move notification click event dispatching out of ServiceWorkerVersion.

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 96c54453043cc695265eb67c440b6f02a2f8f172..933a968264ba8b1583f1c29bbc1ac9394b6e3b80 100644
--- a/content/browser/service_worker/service_worker_version.h
+++ b/content/browser/service_worker/service_worker_version.h
@@ -52,7 +52,6 @@ class ServiceWorkerProviderHost;
 class ServiceWorkerRegistration;
 class ServiceWorkerURLRequestJob;
 struct NavigatorConnectClient;
-struct PlatformNotificationData;
 struct ServiceWorkerClientInfo;
 struct ServiceWorkerVersionInfo;
 struct TransferredMessagePort;
@@ -259,17 +258,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
                           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.
@@ -403,7 +391,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
     REQUEST_ACTIVATE,
     REQUEST_INSTALL,
     REQUEST_FETCH,
-    REQUEST_NOTIFICATION_CLICK,
     REQUEST_PUSH,
     REQUEST_CUSTOM,
     NUM_REQUEST_TYPES
@@ -565,7 +552,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
   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);
@@ -694,8 +680,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
   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_;
 
@@ -818,12 +802,16 @@ bool ServiceWorkerVersion::EventResponseHandler<ResponseMessage, CallbackType>::
   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;
 }