[ServiceWorker] Make ServiceWorkerDispatcherHost not a BrowserMessageFilter any more

As Service Worker IPCs are all in Mojo now, we no longer use
ServiceWorkerDispatcherHost to handle any legacy IPCs, its only usage is
to implement mojom::ServiceWorkerDispatcherHost interface, so, it no
longer needs to be a BrowserMessageFilter.
This CL makes ServiceWorkerDispatcherHost not a BrowserMessageFilter any
more, but still keeps its Mojo interface being associated with the
legacy IPC channel.

In future once we make clear of those potential races with some legacy
IPCs like navigation IPCs, we can consider putting
mojom::ServiceWorkerDispatcherHost on a dedicated Mojo message pipe, or
we'd remove mojom::ServiceWorkerDispatcherHost completely if we can find
an alternative/clear way to achieve the same goal.

BUG=845341

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I9f764017832a5e2f9ff15aff9b49b4c2a6834b78
Reviewed-on: https://chromium-review.googlesource.com/1105623Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Han Leon <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#569232}

content/browser/bad_message.h

@@ -199,9 +199,9 @@ enum BadMessageReason {
   RFH_BASE_URL_FOR_DATA_URL_SPECIFIED = 171,
   RFPH_ILLEGAL_UPLOAD_PARAMS = 172,
   OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE = 173,
-  SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_NOT_WINDOW = 174,
-  SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_SERVICE_WORKER = 175,
-  SWDH_PROVIDER_CREATED_DUPLICATE_ID = 176,
+  OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_NOT_WINDOW = 174,
+  OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_SERVICE_WORKER = 175,
+  OBSOLETE_SWDH_PROVIDER_CREATED_DUPLICATE_ID = 176,
   OBSOLETE_SWDH_PROVIDER_CREATED_BAD_ID = 177,
   RFH_KEEP_ALIVE_HANDLE_REQUESTED_INCORRECTLY = 178,
   BFSI_INVALID_UNIQUE_ID = 179,

content/browser/renderer_host/render_process_host_impl.cc

@@ -1413,6 +1413,9 @@ RenderProcessHostImpl::RenderProcessHostImpl(
           storage_partition_impl_->GetURLRequestContext(),
           storage_partition_impl_->GetIndexedDBContext(),
           ChromeBlobStorageContext::GetFor(browser_context_))),
+      service_worker_dispatcher_host_(new ServiceWorkerDispatcherHost(
+          storage_partition_impl_->GetServiceWorkerContext(),
+          id_)),
       channel_connected_(false),
       sent_render_process_ready_(false),
 #if defined(OS_ANDROID)
@@ -1449,6 +1452,7 @@ RenderProcessHostImpl::RenderProcessHostImpl(
       GetID(), storage_partition_impl_->GetServiceWorkerContext()));
 
   AddObserver(indexed_db_factory_.get());
+  AddObserver(service_worker_dispatcher_host_.get());
 #if defined(OS_MACOSX)
   AddObserver(MachBroker::GetInstance());
 #endif
@@ -1840,11 +1844,6 @@ void RenderProcessHostImpl::CreateMessageFilters() {
   AddFilter(new TextInputClientMessageFilter());
 #endif
 
-  auto service_worker_filter =
-      base::MakeRefCounted<ServiceWorkerDispatcherHost>(
-          storage_partition_impl_->GetServiceWorkerContext(), GetID());
-  AddFilter(service_worker_filter.get());
-
   p2p_socket_dispatcher_host_ = new P2PSocketDispatcherHost(
       resource_context, request_context.get());
   AddFilter(p2p_socket_dispatcher_host_.get());
@@ -1905,6 +1904,10 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
       base::Bind(&IndexedDBDispatcherHost::AddBinding,
                  base::Unretained(indexed_db_factory_.get())));
 
+  channel_->AddAssociatedInterfaceForIOThread(base::BindRepeating(
+      &ServiceWorkerDispatcherHost::AddBinding,
+      base::Unretained(service_worker_dispatcher_host_.get())));
+
   AddUIThreadInterface(
       registry.get(), base::Bind(&ForwardRequest<device::mojom::BatteryMonitor>,
                                  device::mojom::kServiceName));

content/browser/renderer_host/render_process_host_impl.h

@@ -87,6 +87,7 @@ class RenderWidgetHelper;
 class RenderWidgetHost;
 class RenderWidgetHostImpl;
 class ResourceMessageFilter;
+class ServiceWorkerDispatcherHost;
 class SiteInstance;
 class SiteInstanceImpl;
 class StoragePartition;
@@ -794,6 +795,9 @@ class CONTENT_EXPORT RenderProcessHostImpl
   std::unique_ptr<IndexedDBDispatcherHost, BrowserThread::DeleteOnIOThread>
       indexed_db_factory_;
 
+  std::unique_ptr<ServiceWorkerDispatcherHost, BrowserThread::DeleteOnIOThread>
+      service_worker_dispatcher_host_;
+
   scoped_refptr<CacheStorageDispatcherHost> cache_storage_dispatcher_host_;
 
   bool channel_connected_;

content/browser/service_worker/service_worker_dispatcher_host.cc

@@ -6,94 +6,46 @@
 
 #include <utility>
 
-#include "base/logging.h"
-#include "base/macros.h"
-#include "base/memory/ptr_util.h"
-#include "base/strings/string_number_conversions.h"
-#include "base/strings/utf_string_conversions.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
-#include "content/browser/bad_message.h"
-#include "content/browser/service_worker/embedded_worker_status.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
-#include "content/browser/service_worker/service_worker_navigation_handle_core.h"
-#include "content/browser/service_worker/service_worker_object_host.h"
-#include "content/browser/service_worker/service_worker_registration.h"
-#include "content/common/service_worker/service_worker_messages.h"
 #include "content/common/service_worker/service_worker_utils.h"
-#include "content/public/browser/browser_thread.h"
-#include "content/public/browser/content_browser_client.h"
-#include "content/public/browser/render_frame_host.h"
-#include "content/public/browser/web_contents.h"
-#include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/child_process_host.h"
-#include "content/public/common/content_client.h"
-#include "content/public/common/origin_util.h"
-#include "ipc/ipc_message_macros.h"
-#include "third_party/blink/public/mojom/service_worker/service_worker_error_type.mojom.h"
-#include "third_party/blink/public/mojom/service_worker/service_worker_object.mojom.h"
 #include "third_party/blink/public/mojom/service_worker/service_worker_provider_type.mojom.h"
-#include "third_party/blink/public/platform/modules/serviceworker/web_service_worker_error.h"
-#include "url/gurl.h"
-
-using blink::MessagePortChannel;
-using blink::WebServiceWorkerError;
 
 namespace content {
 
-namespace {
-
-const uint32_t kServiceWorkerFilteredMessageClasses[] = {
-    ServiceWorkerMsgStart,
-};
-
-void RemoveAllProviderHostsForProcess(ServiceWorkerContextCore* context,
-                                      int render_process_id) {
-  // TODO(falken) Try to remove this call. It should be unnecessary because
-  // provider hosts remove themselves when their Mojo connection to the
-  // renderer is destroyed. But if we don't remove the hosts immediately here,
-  // collisions of <process_id, provider_id> can occur if a new dispatcher
-  // host is created for a reused RenderProcessHost. https://crbug.com/736203
-  if (context) {
-    context->RemoveAllProviderHostsForProcess(render_process_id);
-  }
-}
-
-}  // namespace
-
 ServiceWorkerDispatcherHost::ServiceWorkerDispatcherHost(
     scoped_refptr<ServiceWorkerContextWrapper> context_wrapper,
     int render_process_id)
-    : BrowserMessageFilter(kServiceWorkerFilteredMessageClasses,
-                           arraysize(kServiceWorkerFilteredMessageClasses)),
-      BrowserAssociatedInterface<mojom::ServiceWorkerDispatcherHost>(this,
-                                                                     this),
-      render_process_id_(render_process_id),
-      context_wrapper_(context_wrapper) {}
+    : render_process_id_(render_process_id), context_wrapper_(context_wrapper) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
 
 ServiceWorkerDispatcherHost::~ServiceWorkerDispatcherHost() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  RemoveAllProviderHostsForProcess(GetContext(), render_process_id_);
 }
 
-void ServiceWorkerDispatcherHost::OnFilterRemoved() {
+void ServiceWorkerDispatcherHost::AddBinding(
+    mojom::ServiceWorkerDispatcherHostAssociatedRequest request) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  // Don't wait until the destructor to teardown since a new dispatcher host
-  // for this process might be created before then.
-  RemoveAllProviderHostsForProcess(GetContext(), render_process_id_);
-  context_wrapper_ = nullptr;
-}
-
-void ServiceWorkerDispatcherHost::OnDestruct() const {
-  // Destruct on the IO thread since |context_wrapper_| should only be accessed
-  // on the IO thread.
-  BrowserThread::DeleteOnIOThread::Destruct(this);
+  bindings_.AddBinding(this, std::move(request));
 }
 
-bool ServiceWorkerDispatcherHost::OnMessageReceived(
-    const IPC::Message& message) {
-  return false;
+void ServiceWorkerDispatcherHost::RenderProcessExited(
+    RenderProcessHost* host,
+    const ChildProcessTerminationInfo& info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // TODO(crbug.com/736203) Try to remove this call. It should be unnecessary
+  // because provider hosts remove themselves when their Mojo connection to the
+  // renderer is destroyed. But if we don't remove the hosts immediately here,
+  // collisions of <process_id, provider_id> can occur if |this| is reused for
+  // another new renderer process due to reuse of the RenderProcessHost.
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::BindOnce(
+          &ServiceWorkerDispatcherHost::RemoveAllProviderHostsForProcess,
+          base::Unretained(this)));
 }
 
 void ServiceWorkerDispatcherHost::OnProviderCreated(
@@ -101,11 +53,11 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerDispatcherHost::OnProviderCreated");
-  if (!GetContext())
+  ServiceWorkerContextCore* context = context_wrapper_->context();
+  if (!context)
     return;
-  if (GetContext()->GetProviderHost(render_process_id_, info.provider_id)) {
-    bad_message::ReceivedBadMessage(
-        this, bad_message::SWDH_PROVIDER_CREATED_DUPLICATE_ID);
+  if (context->GetProviderHost(render_process_id_, info.provider_id)) {
+    bindings_.ReportBadMessage("SWDH_PROVIDER_CREATED_DUPLICATE_ID");
     return;
   }
 
@@ -114,27 +66,27 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
   // creates the provider.
   if (ServiceWorkerUtils::IsBrowserAssignedProviderId(info.provider_id)) {
     if (info.type != blink::mojom::ServiceWorkerProviderType::kForWindow) {
-      bad_message::ReceivedBadMessage(
-          this, bad_message::SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_NOT_WINDOW);
+      bindings_.ReportBadMessage(
+          "SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_NOT_WINDOW");
       return;
     }
 
     // Retrieve the provider host pre-created for the navigation.
     std::unique_ptr<ServiceWorkerProviderHost> provider_host =
-        GetContext()->ReleaseProviderHost(ChildProcessHost::kInvalidUniqueID,
-                                          info.provider_id);
+        context->ReleaseProviderHost(ChildProcessHost::kInvalidUniqueID,
+                                     info.provider_id);
     // If no host is found, create one.
     // TODO(crbug.com/789111#c14): This is probably not right, see bug.
     if (!provider_host) {
-      GetContext()->AddProviderHost(ServiceWorkerProviderHost::Create(
-          render_process_id_, std::move(info), GetContext()->AsWeakPtr()));
+      context->AddProviderHost(ServiceWorkerProviderHost::Create(
+          render_process_id_, std::move(info), context->AsWeakPtr()));
       return;
     }
 
     // Otherwise, complete initialization of the pre-created host.
     provider_host->CompleteNavigationInitialized(render_process_id_,
                                                  std::move(info));
-    GetContext()->AddProviderHost(std::move(provider_host));
+    context->AddProviderHost(std::move(provider_host));
     return;
   }
 
@@ -142,20 +94,21 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
   // precreated and ServiceWorkerProviderHost::CompleteStartWorkerPreparation is
   // called during the startup sequence once a process is allocated.
   if (info.type == blink::mojom::ServiceWorkerProviderType::kForServiceWorker) {
-    bad_message::ReceivedBadMessage(
-        this, bad_message::SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_SERVICE_WORKER);
+    bindings_.ReportBadMessage(
+        "SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_SERVICE_WORKER");
     return;
   }
 
-  GetContext()->AddProviderHost(ServiceWorkerProviderHost::Create(
-      render_process_id_, std::move(info), GetContext()->AsWeakPtr()));
+  context->AddProviderHost(ServiceWorkerProviderHost::Create(
+      render_process_id_, std::move(info), context->AsWeakPtr()));
 }
 
-ServiceWorkerContextCore* ServiceWorkerDispatcherHost::GetContext() {
+void ServiceWorkerDispatcherHost::RemoveAllProviderHostsForProcess() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (!context_wrapper_.get())
-    return nullptr;
-  return context_wrapper_->context();
+  if (context_wrapper_->context()) {
+    context_wrapper_->context()->RemoveAllProviderHostsForProcess(
+        render_process_id_);
+  }
 }
 
 }  // namespace content

content/browser/service_worker/service_worker_dispatcher_host.h

@@ -7,24 +7,16 @@
 
 #include <stdint.h>
 
-#include <memory>
-#include <vector>
-
 #include "base/macros.h"
-#include "base/optional.h"
-#include "base/strings/string16.h"
-#include "content/browser/service_worker/service_worker_registration_status.h"
+#include "content/common/content_export.h"
 #include "content/common/service_worker/service_worker.mojom.h"
-#include "content/common/service_worker/service_worker_event_dispatcher.mojom.h"
 #include "content/common/service_worker/service_worker_types.h"
-#include "content/public/browser/browser_associated_interface.h"
-#include "content/public/browser/browser_message_filter.h"
-#include "mojo/public/cpp/bindings/strong_associated_binding_set.h"
-#include "third_party/blink/public/mojom/service_worker/service_worker_registration.mojom.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_process_host_observer.h"
+#include "mojo/public/cpp/bindings/associated_binding_set.h"
 
 namespace content {
 
-class ServiceWorkerContextCore;
 class ServiceWorkerContextWrapper;
 
 namespace service_worker_dispatcher_host_unittest {
@@ -36,31 +28,28 @@ FORWARD_DECLARE_TEST(ServiceWorkerDispatcherHostTest, CleanupOnRendererCrash);
 }  // namespace service_worker_dispatcher_host_unittest
 
 // ServiceWorkerDispatcherHost is a browser-side endpoint for the renderer to
-// notify the browser ServiceWorkerProviderHost is created. In order to
-// associate the Mojo interface with the legacy IPC channel,
-// ServiceWorkerDispatcherHost overrides BrowserMessageFilter and
-// BrowserAssociatedInterface.
-//
-// ServiceWorkerDispatcherHost is created on the UI thread in
-// RenderProcessHostImpl::Init() via CreateMessageFilters(), but initialization,
-// destruction, and Mojo calls occur on the IO thread. It lives as
-// long as the renderer process lives.
+// notify the browser a service worker provider is created.
+// Unless otherwise noted, all methods are called on the IO thread.
 //
+// In order to keep ordering with navigation IPCs to avoid potential races,
+// currently mojom::ServiceWorkerDispatcherHost interface is associated with the
+// legacy IPC channel.
 // TODO(leonhsl): Remove this class once we can understand how to move
 // OnProviderCreated() to an isolated message pipe.
 class CONTENT_EXPORT ServiceWorkerDispatcherHost
-    : public BrowserMessageFilter,
-      public BrowserAssociatedInterface<mojom::ServiceWorkerDispatcherHost>,
-      public mojom::ServiceWorkerDispatcherHost {
+    : public mojom::ServiceWorkerDispatcherHost,
+      public RenderProcessHostObserver {
  public:
+  // Called on the UI thread.
   ServiceWorkerDispatcherHost(
       scoped_refptr<ServiceWorkerContextWrapper> context_wrapper,
       int render_process_id);
 
-  // BrowserMessageFilter implementation
-  void OnFilterRemoved() override;
-  void OnDestruct() const override;
-  bool OnMessageReceived(const IPC::Message& message) override;
+  void AddBinding(mojom::ServiceWorkerDispatcherHostAssociatedRequest request);
+
+  // Called on the UI thread.
+  void RenderProcessExited(RenderProcessHost* host,
+                           const ChildProcessTerminationInfo& info) override;
 
  protected:
   ~ServiceWorkerDispatcherHost() override;
@@ -82,11 +71,12 @@ class CONTENT_EXPORT ServiceWorkerDispatcherHost
   // mojom::ServiceWorkerDispatcherHost implementation
   void OnProviderCreated(ServiceWorkerProviderHostInfo info) override;
 
-  ServiceWorkerContextCore* GetContext();
+  void RemoveAllProviderHostsForProcess();
 
   const int render_process_id_;
   // Only accessed on the IO thread.
   scoped_refptr<ServiceWorkerContextWrapper> context_wrapper_;
+  mojo::AssociatedBindingSet<mojom::ServiceWorkerDispatcherHost> bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerDispatcherHost);
 };

tools/metrics/histograms/enums.xml

@@ -3175,9 +3175,11 @@ uploading your change for review.  These are checked by presubmit scripts.
   <int value="171" label="RFH_BASE_URL_FOR_DATA_URL_SPECIFIED"/>
   <int value="172" label="RFPH_ILLEGAL_UPLOAD_PARAMS"/>
   <int value="173" label="OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE"/>
-  <int value="174" label="SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_NOT_WINDOW"/>
-  <int value="175" label="SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_SERVICE_WORKER"/>
-  <int value="176" label="SWDH_PROVIDER_CREATED_DUPLICATE_ID"/>
+  <int value="174"
+      label="OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_NOT_WINDOW"/>
+  <int value="175"
+      label="OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_SERVICE_WORKER"/>
+  <int value="176" label="OBSOLETE_SWDH_PROVIDER_CREATED_DUPLICATE_ID"/>
   <int value="177" label="OBSOLETE_SWDH_PROVIDER_CREATED_BAD_ID"/>
   <int value="178" label="RFH_KEEP_ALIVE_HANDLE_REQUESTED_INCORRECTLY"/>
   <int value="179" label="BFSI_INVALID_UNIQUE_ID"/>