[ServiceWorker] ServiceWorkerContextCore no longer needs to manage SWDispatcherHosts

After https://chromium-review.googlesource.com/c/chromium/src/+/1068514,
no code will try to find the specific ServiceWorkerDispatcherHost for a
given renderer process, so this CL removes the mapping management of
ServiceWorkerDispatcherHosts inside ServiceWorkerContextCore, in the
meantime simplifies ServiceWorkerDispatcherHost a little.

BUG=845341

Change-Id: Ie7747846570f64a7a4eda960878bbc0c97042952
Reviewed-on: https://chromium-review.googlesource.com/1075849Reviewed-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@{#563090}

content/browser/background_sync/background_sync_manager_unittest.cc

@@ -25,7 +25,6 @@
 #include "content/browser/service_worker/embedded_worker_test_helper.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_dispatcher_host.h"
 #include "content/browser/service_worker/service_worker_registration_object_host.h"
 #include "content/browser/service_worker/service_worker_storage.h"
 #include "content/browser/storage_partition_impl.h"

content/browser/renderer_host/render_process_host_impl.cc

@@ -1839,9 +1839,8 @@ void RenderProcessHostImpl::CreateMessageFilters() {
 #endif
 
   auto service_worker_filter =
-      base::MakeRefCounted<ServiceWorkerDispatcherHost>(GetID());
-  service_worker_filter->Init(
-      storage_partition_impl_->GetServiceWorkerContext());
+      base::MakeRefCounted<ServiceWorkerDispatcherHost>(
+          storage_partition_impl_->GetServiceWorkerContext(), GetID());
   AddFilter(service_worker_filter.get());
 
   p2p_socket_dispatcher_host_ = new P2PSocketDispatcherHost(

content/browser/service_worker/service_worker_context_core.cc

@@ -23,7 +23,6 @@
 #include "content/browser/service_worker/embedded_worker_status.h"
 #include "content/browser/service_worker/service_worker_context_core_observer.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
-#include "content/browser/service_worker/service_worker_dispatcher_host.h"
 #include "content/browser/service_worker/service_worker_info.h"
 #include "content/browser/service_worker/service_worker_job_coordinator.h"
 #include "content/browser/service_worker/service_worker_process_manager.h"
@@ -299,7 +298,6 @@ ServiceWorkerContextCore::ServiceWorkerContextCore(
     ServiceWorkerContextCore* old_context,
     ServiceWorkerContextWrapper* wrapper)
     : wrapper_(wrapper),
-      dispatcher_hosts_(std::move(old_context->dispatcher_hosts_)),
       providers_(old_context->providers_.release()),
       provider_by_uuid_(old_context->provider_by_uuid_.release()),
       loader_factory_getter_(old_context->loader_factory_getter()),
@@ -325,32 +323,6 @@ ServiceWorkerContextCore::~ServiceWorkerContextCore() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
-void ServiceWorkerContextCore::AddDispatcherHost(
-    int process_id,
-    content::ServiceWorkerDispatcherHost* dispatcher_host) {
-  DCHECK(dispatcher_hosts_.find(process_id) == dispatcher_hosts_.end());
-  dispatcher_hosts_[process_id] = dispatcher_host;
-}
-
-ServiceWorkerDispatcherHost* ServiceWorkerContextCore::GetDispatcherHost(
-    int process_id) {
-  auto it = dispatcher_hosts_.find(process_id);
-  if (it == dispatcher_hosts_.end())
-    return nullptr;
-  return it->second;
-}
-
-void ServiceWorkerContextCore::RemoveDispatcherHost(int process_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  // 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
-  RemoveAllProviderHostsForProcess(process_id);
-  dispatcher_hosts_.erase(process_id);
-}
-
 void ServiceWorkerContextCore::AddProviderHost(
     std::unique_ptr<ServiceWorkerProviderHost> host) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

content/browser/service_worker/service_worker_context_core.h

@@ -43,7 +43,6 @@ namespace content {
 class EmbeddedWorkerRegistry;
 class ServiceWorkerContextCoreObserver;
 class ServiceWorkerContextWrapper;
-class ServiceWorkerDispatcherHost;
 class ServiceWorkerJobCoordinator;
 class ServiceWorkerProviderHost;
 class ServiceWorkerRegistration;
@@ -160,13 +159,6 @@ class CONTENT_EXPORT ServiceWorkerContextCore
     return job_coordinator_.get();
   }
 
-  // Maintains DispatcherHosts to exchange service worker related messages
-  // through them. The DispatcherHosts are not owned by this class.
-  void AddDispatcherHost(int process_id,
-                         ServiceWorkerDispatcherHost* dispatcher_host);
-  ServiceWorkerDispatcherHost* GetDispatcherHost(int process_id);
-  void RemoveDispatcherHost(int process_id);
-
   // The context class owns the set of ProviderHosts.
   void AddProviderHost(
       std::unique_ptr<ServiceWorkerProviderHost> provider_host);
@@ -339,8 +331,6 @@ class CONTENT_EXPORT ServiceWorkerContextCore
   // because the Wrapper::Shutdown call that hops threads to destroy |this| uses
   // Bind() to hold a reference to |wrapper_| until |this| is fully destroyed.
   ServiceWorkerContextWrapper* wrapper_;
-  std::map<int /* process_id */, ServiceWorkerDispatcherHost*>
-      dispatcher_hosts_;
   std::unique_ptr<ProcessToProviderMap> providers_;
   std::unique_ptr<ProviderByClientUUIDMap> provider_by_uuid_;
   std::unique_ptr<ServiceWorkerStorage> storage_;

content/browser/service_worker/service_worker_dispatcher_host.cc

@@ -48,51 +48,40 @@ 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(int render_process_id)
+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) {}
+      render_process_id_(render_process_id),
+      context_wrapper_(context_wrapper) {}
 
 ServiceWorkerDispatcherHost::~ServiceWorkerDispatcherHost() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (GetContext() && phase_ == Phase::kAddedToContext)
-    GetContext()->RemoveDispatcherHost(render_process_id_);
-}
-
-void ServiceWorkerDispatcherHost::Init(
-    ServiceWorkerContextWrapper* context_wrapper) {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        base::BindOnce(&ServiceWorkerDispatcherHost::Init, this,
-                       base::RetainedRef(context_wrapper)));
-    return;
-  }
-
-  // Just speculating that maybe we were destructed before Init() was called on
-  // the IO thread in order to try to fix https://crbug.com/750267.
-  if (phase_ != Phase::kInitial)
-    return;
-
-  context_wrapper_ = context_wrapper;
-  if (!GetContext())
-    return;
-  GetContext()->AddDispatcherHost(render_process_id_, this);
-  phase_ = Phase::kAddedToContext;
+  RemoveAllProviderHostsForProcess(GetContext(), render_process_id_);
 }
 
 void ServiceWorkerDispatcherHost::OnFilterRemoved() {
   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.
-  if (GetContext() && phase_ == Phase::kAddedToContext) {
-    GetContext()->RemoveDispatcherHost(render_process_id_);
-  }
-  phase_ = Phase::kRemovedFromContext;
+  RemoveAllProviderHostsForProcess(GetContext(), render_process_id_);
   context_wrapper_ = nullptr;
 }
 
@@ -109,6 +98,7 @@ bool ServiceWorkerDispatcherHost::OnMessageReceived(
 
 void ServiceWorkerDispatcherHost::OnProviderCreated(
     ServiceWorkerProviderHostInfo info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerDispatcherHost::OnProviderCreated");
   if (!GetContext())

content/browser/service_worker/service_worker_dispatcher_host.h

@@ -33,40 +33,29 @@ class TestingServiceWorkerDispatcherHost;
 FORWARD_DECLARE_TEST(ServiceWorkerDispatcherHostTest,
                      ProviderCreatedAndDestroyed);
 FORWARD_DECLARE_TEST(ServiceWorkerDispatcherHostTest, CleanupOnRendererCrash);
-FORWARD_DECLARE_TEST(BackgroundSyncManagerTest,
-                     RegisterWithoutLiveSWRegistration);
 }  // namespace service_worker_dispatcher_host_unittest
 
-// ServiceWorkerDispatcherHost is the browser-side endpoint for several IPC
-// messages for service workers. There is a 1:1 correspondence between
-// renderer processes and ServiceWorkerDispatcherHosts. Currently
-// ServiceWorkerDispatcherHost sends the legacy IPC message
-// ServiceWorkerMsg_ServiceWorkerStateChanged to its corresponding
-// ServiceWorkerDispatcher on the renderer and receives Mojo IPC messages from
-// any ServiceWorkerNetworkProvider on the renderer.
+// 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 IPC message handling occur on the IO thread. It lives as
-// long as the renderer process lives. Therefore much tracking of renderer
-// processes in browser-side service worker code is built on
-// ServiceWorkerDispatcherHost lifetime.
+// destruction, and Mojo calls occur on the IO thread. It lives as
+// long as the renderer process lives.
 //
-// This class is bound with mojom::ServiceWorkerDispatcherHost. All
-// InterfacePtrs on the same render process are bound to the same
-// content::ServiceWorkerDispatcherHost. This can be overridden only for
-// testing.
-//
-// TODO(leonhsl): This class no longer needs to be a BrowserMessageFilter
-// because we are already in a pure Mojo world.
+// 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:
-  explicit ServiceWorkerDispatcherHost(int render_process_id);
-
-  void Init(ServiceWorkerContextWrapper* context_wrapper);
+  ServiceWorkerDispatcherHost(
+      scoped_refptr<ServiceWorkerContextWrapper> context_wrapper,
+      int render_process_id);
 
   // BrowserMessageFilter implementation
   void OnFilterRemoved() override;
@@ -89,13 +78,6 @@ class CONTENT_EXPORT ServiceWorkerDispatcherHost
   FRIEND_TEST_ALL_PREFIXES(
       service_worker_dispatcher_host_unittest::ServiceWorkerDispatcherHostTest,
       CleanupOnRendererCrash);
-  FRIEND_TEST_ALL_PREFIXES(
-      service_worker_dispatcher_host_unittest::BackgroundSyncManagerTest,
-      RegisterWithoutLiveSWRegistration);
-
-  enum class ProviderStatus { OK, NO_CONTEXT, DEAD_HOST, NO_HOST, NO_URL };
-  // Debugging for https://crbug.com/750267
-  enum class Phase { kInitial, kAddedToContext, kRemovedFromContext };
 
   // mojom::ServiceWorkerDispatcherHost implementation
   void OnProviderCreated(ServiceWorkerProviderHostInfo info) override;
@@ -104,8 +86,6 @@ class CONTENT_EXPORT ServiceWorkerDispatcherHost
 
   const int render_process_id_;
   // Only accessed on the IO thread.
-  Phase phase_ = Phase::kInitial;
-  // Only accessed on the IO thread.
   scoped_refptr<ServiceWorkerContextWrapper> context_wrapper_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerDispatcherHost);

content/browser/service_worker/service_worker_dispatcher_host_unittest.cc

@@ -85,9 +85,11 @@ std::unique_ptr<ServiceWorkerNavigationHandleCore> CreateNavigationHandleCore(
 
 class TestingServiceWorkerDispatcherHost : public ServiceWorkerDispatcherHost {
  public:
-  TestingServiceWorkerDispatcherHost(int process_id,
-                                     EmbeddedWorkerTestHelper* helper)
-      : ServiceWorkerDispatcherHost(process_id),
+  TestingServiceWorkerDispatcherHost(
+      scoped_refptr<ServiceWorkerContextWrapper> context_wrapper,
+      int process_id,
+      EmbeddedWorkerTestHelper* helper)
+      : ServiceWorkerDispatcherHost(context_wrapper, process_id),
         bad_messages_received_count_(0),
         helper_(helper) {}
 
@@ -128,9 +130,8 @@ class ServiceWorkerDispatcherHostTest : public testing::Test {
     helper_.reset(helper.release());
     // Replace the default dispatcher host.
     int process_id = helper_->mock_render_process_id();
-    dispatcher_host_ =
-        new TestingServiceWorkerDispatcherHost(process_id, helper_.get());
-    dispatcher_host_->Init(context_wrapper());
+    dispatcher_host_ = new TestingServiceWorkerDispatcherHost(
+        context_wrapper(), process_id, helper_.get());
   }
 
   void SetUpRegistration(const GURL& scope, const GURL& script_url) {
@@ -282,9 +283,8 @@ TEST_F(ServiceWorkerDispatcherHostTest, CleanupOnRendererCrash) {
   // is not yet destroyed. This is what the browser does when reusing a crashed
   // render process.
   auto new_dispatcher_host =
-      base::MakeRefCounted<TestingServiceWorkerDispatcherHost>(process_id,
-                                                               helper_.get());
-  new_dispatcher_host->Init(context_wrapper());
+      base::MakeRefCounted<TestingServiceWorkerDispatcherHost>(
+          context_wrapper(), process_id, helper_.get());
 
   // To show the new dispatcher can operate, simulate provider creation. Since
   // the old dispatcher cleaned up the old provider host, the new one won't