ServiceWorker: Release a reference when it fails to dispatch ExtendableMessageEvent

content/browser/service_worker/service_worker_dispatcher_host.cc

 // 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.
 
 #include "content/browser/service_worker/service_worker_dispatcher_host.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/macros.h"
@@ skipping 683 matching lines @@
     return;
   }
 
   ServiceWorkerProviderHost* sender_provider_host =
       GetContext()->GetProviderHost(render_process_id_, provider_id);
   if (!sender_provider_host) {
     bad_message::ReceivedBadMessage(this, bad_message::SWDH_POST_MESSAGE);
     return;
   }
 
+  DispatchExtendableMessageEvent(
+      make_scoped_refptr(handle->version()), message, source_origin,
+      sent_message_ports, sender_provider_host,
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+}
+
+void ServiceWorkerDispatcherHost::DispatchExtendableMessageEvent(
+    scoped_refptr<ServiceWorkerVersion> worker,
+    const base::string16& message,
+    const url::Origin& source_origin,
+    const std::vector<TransferredMessagePort>& sent_message_ports,
+    ServiceWorkerProviderHost* sender_provider_host,
+    const StatusCallback& callback) {
   for (const TransferredMessagePort& port : sent_message_ports)
     MessagePortService::GetInstance()->HoldMessages(port.id);
 
   switch (sender_provider_host->provider_type()) {
     case SERVICE_WORKER_PROVIDER_FOR_WINDOW:
     case SERVICE_WORKER_PROVIDER_FOR_WORKER:
     case SERVICE_WORKER_PROVIDER_FOR_SHARED_WORKER:
       service_worker_client_utils::GetClient(
           sender_provider_host,
-          base::Bind(
-              &ServiceWorkerDispatcherHost::DispatchExtendableMessageEvent<
-                  ServiceWorkerClientInfo>,
-              this, make_scoped_refptr(handle->version()), message,
-              source_origin, sent_message_ports));
+          base::Bind(&ServiceWorkerDispatcherHost::
+                         DispatchExtendableMessageEventInternal<
+                             ServiceWorkerClientInfo>,
+                     this, worker, message, source_origin, sent_message_ports,
+                     callback));
       break;
     case SERVICE_WORKER_PROVIDER_FOR_CONTROLLER:
-      // TODO(nhiroki): Decrement a reference to ServiceWorkerHandle if starting
-      // worker fails (http://crbug.com/543198).
       RunSoon(base::Bind(
-          &ServiceWorkerDispatcherHost::DispatchExtendableMessageEvent<
+          &ServiceWorkerDispatcherHost::DispatchExtendableMessageEventInternal<
               ServiceWorkerObjectInfo>,
-          this, make_scoped_refptr(handle->version()), message, source_origin,
-          sent_message_ports,
+          this, worker, message, source_origin, sent_message_ports, callback,
           sender_provider_host->GetOrCreateServiceWorkerHandle(
               sender_provider_host->running_hosted_version())));
       break;
     case SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME:
     case SERVICE_WORKER_PROVIDER_UNKNOWN:
       NOTREACHED() << sender_provider_host->provider_type();
       break;
   }
 }
 
@@ skipping 122 matching lines @@
   ServiceWorkerRegistrationObjectInfo info;
   ServiceWorkerVersionAttributes attrs;
   GetRegistrationObjectInfoAndVersionAttributes(
       provider_host->AsWeakPtr(), registration, &info, &attrs);
 
   Send(new ServiceWorkerMsg_AssociateRegistration(kDocumentMainThreadId,
                                                   provider_id, info, attrs));
 }
 
 template <typename SourceInfo>
-void ServiceWorkerDispatcherHost::DispatchExtendableMessageEvent(
+void ServiceWorkerDispatcherHost::DispatchExtendableMessageEventInternal(
     scoped_refptr<ServiceWorkerVersion> worker,
     const base::string16& message,
     const url::Origin& source_origin,
     const std::vector<TransferredMessagePort>& sent_message_ports,
+    const StatusCallback& callback,
     const SourceInfo& source_info) {
   if (!source_info.IsValid()) {
-    DidFailToDispatchExtendableMessageEvent(sent_message_ports,
-                                            SERVICE_WORKER_ERROR_FAILED);
+    DidFailToDispatchExtendableMessageEvent<SourceInfo>(
+        sent_message_ports, source_info, callback, SERVICE_WORKER_ERROR_FAILED);
     return;
   }
   worker->RunAfterStartWorker(
       base::Bind(&ServiceWorkerDispatcherHost::
                      DispatchExtendableMessageEventAfterStartWorker,
                  this, worker, message, source_origin, sent_message_ports,
-                 ExtendableMessageEventSource(source_info)),
+                 ExtendableMessageEventSource(source_info), callback),
       base::Bind(
-          &ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent,
-          this, sent_message_ports));
+          &ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent<
+              SourceInfo>,
+          this, sent_message_ports, source_info, callback));
 }
 
 void ServiceWorkerDispatcherHost::
     DispatchExtendableMessageEventAfterStartWorker(
         scoped_refptr<ServiceWorkerVersion> worker,
         const base::string16& message,
         const url::Origin& source_origin,
         const std::vector<TransferredMessagePort>& sent_message_ports,
-        const ExtendableMessageEventSource& source) {
+        const ExtendableMessageEventSource& source,
+        const StatusCallback& callback) {
   int request_id =
-      worker->StartRequest(ServiceWorkerMetrics::EventType::MESSAGE,
-                           base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+      worker->StartRequest(ServiceWorkerMetrics::EventType::MESSAGE, callback);
 
   MessagePortMessageFilter* filter =
       worker->embedded_worker()->message_port_message_filter();
   std::vector<int> new_routing_ids;
   filter->UpdateMessagePortsWithNewRoutes(sent_message_ports, &new_routing_ids);
 
   ServiceWorkerMsg_ExtendableMessageEvent_Params params;
   params.message = message;
   params.source_origin = source_origin;
   params.message_ports = sent_message_ports;
   params.new_routing_ids = new_routing_ids;
   params.source = source;
 
   // Hide the client url if the client has a unique origin.
   if (source_origin.unique()) {
     if (params.source.client_info.IsValid())
       params.source.client_info.url = GURL();
     else
       params.source.service_worker_info.url = GURL();
   }
 
   worker->DispatchSimpleEvent<
       ServiceWorkerHostMsg_ExtendableMessageEventFinished>(
       request_id, ServiceWorkerMsg_ExtendableMessageEvent(request_id, params));
 }
 
+template <typename SourceInfo>
 void ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent(
     const std::vector<TransferredMessagePort>& sent_message_ports,
+    const SourceInfo& source_info,
+    const StatusCallback& callback,
     ServiceWorkerStatusCode status) {
   // Transfering the message ports failed, so destroy the ports.
   for (const TransferredMessagePort& port : sent_message_ports)
     MessagePortService::GetInstance()->ClosePort(port.id);
+  callback.Run(status);
+}
+
+template <>
+void ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent(

[falken, 2016/03/17 05:10:49]

I'm kind of confused why there are two
ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent's. Can you
help me understand?

[nhiroki, 2016/03/17 06:05:30]

"typename SourceInfo" is resolved to ServiceWorkerObejectInfo or
ServiceWorkerClientInfo.

As for ServiceWorkerObjectInfo, it implicitly has a reference to
ServiceWorkerHandle and needs a special care to release the reference when
sending a message fails.

As for ServiceWorkerClientInfo, it's a kind of snapshot of Client states and
does not have a reference to anything, so the special care is not necessary.

To handle the both cases, I made these two functions. On the second patchset, I
tried to reduce duplicate lines. This could be more explicit about my intention?

[nhiroki, 2016/03/17 06:33:28]

FYI: I tried to use overloaded functions instead of C++ template, but it didn't
work because base::Bind() couldn't determine the functor on compile time.

+    const std::vector<TransferredMessagePort>& sent_message_ports,
+    const ServiceWorkerObjectInfo& source_info,
+    const StatusCallback& callback,
+    ServiceWorkerStatusCode status) {
+  // Transfering the message ports failed, so destroy the ports.
+  for (const TransferredMessagePort& port : sent_message_ports)
+    MessagePortService::GetInstance()->ClosePort(port.id);
+  if (source_info.IsValid()) {
+    ServiceWorkerHandle* handle = handles_.Lookup(source_info.handle_id);
+    DCHECK(handle);
+    handle->DecrementRefCount();
+    if (handle->HasNoRefCount())
+      handles_.Remove(source_info.handle_id);
+  }
+  callback.Run(status);
 }
 
 ServiceWorkerRegistrationHandle*
 ServiceWorkerDispatcherHost::FindRegistrationHandle(int provider_id,
                                                     int64_t registration_id) {
   for (RegistrationHandleMap::iterator iter(&registration_handles_);
        !iter.IsAtEnd(); iter.Advance()) {
     ServiceWorkerRegistrationHandle* handle = iter.GetCurrentValue();
     if (handle->provider_id() == provider_id &&
         handle->registration()->id() == registration_id) {
@@ skipping 447 matching lines @@
   if (!handle) {
     bad_message::ReceivedBadMessage(this,
                                     bad_message::SWDH_TERMINATE_BAD_HANDLE);
     return;
   }
   handle->version()->StopWorker(
       base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
 }
 
 }  // namespace content