service worker: Keep precreated provider hosts in the same process map.

ServiceWorkerContextCore owns provider hosts, and stores them in a map
from <process id, provider id> to host. But precreated hosts are created
before the process id is known. So the hosts are first stored in <-1,
provider id> before being moved to <process id, provider id>. That
complicates adding a Mojo error connection handler to the host, which
tries to call RemoveProviderHost(process_id, provider_id).

Fortunately, precreated hosts have a unique provider id anyway, so the
process id isn't needed. This CL makes it so precreated hosts always
live in <-1, provider id>, so a Mojo error connection handler can be
added to them immediately.

Note that we can't easily make renderer-assigned provider ids unique
since they are generated by each renderer process on its own. But
eventually the renderer-created providers will go away anyway, since
they aren't used since PlzNavigate except for non-S13nServiceWorker
shared worker.

Some more design at:
https://docs.google.com/document/d/1FDaAwQ9O1Z2am1XEMPiEsVzWXgqCkT1ZqpVlcGmBve4/edit

This will allow fixing the linked bug, by the next CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1159544

Bug: 854993
Change-Id: I3f638496e0e7436e484c956531709ff6931418ff
Reviewed-on: https://chromium-review.googlesource.com/1159542
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580102}

content/browser/service_worker/service_worker_context_core.cc

@@ -34,6 +34,7 @@
 #include "content/browser/url_loader_factory_getter.h"
 #include "content/common/service_worker/service_worker_utils.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/common/child_process_host.h"
 #include "ipc/ipc_message.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
@@ -328,6 +329,9 @@ void ServiceWorkerContextCore::AddProviderHost(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   int process_id = host->process_id();
   int provider_id = host->provider_id();
+  // Precreated hosts are stored in the same map regardless of process.
+  if (ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id))
+    process_id = ChildProcessHost::kInvalidUniqueID;
   ProviderMap* map = GetProviderMapForProcess(process_id);
   if (!map) {
     providers_->AddWithID(std::make_unique<ProviderMap>(), process_id);
@@ -336,22 +340,13 @@ void ServiceWorkerContextCore::AddProviderHost(
   map->AddWithID(std::move(host), provider_id);
 }
 
-std::unique_ptr<ServiceWorkerProviderHost>
-ServiceWorkerContextCore::ReleaseProviderHost(int process_id, int provider_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  ProviderMap* map = GetProviderMapForProcess(process_id);
-  if (!map || !map->Lookup(provider_id))
-    return nullptr;
-  std::unique_ptr<ServiceWorkerProviderHost> host =
-      map->Replace(provider_id, nullptr);
-  map->Remove(provider_id);
-  return host;
-}
-
 ServiceWorkerProviderHost* ServiceWorkerContextCore::GetProviderHost(
     int process_id,
     int provider_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // Precreated hosts are stored in the same map regardless of process.
+  if (ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id))
+    process_id = ChildProcessHost::kInvalidUniqueID;
   ProviderMap* map = GetProviderMapForProcess(process_id);
   if (!map)
     return nullptr;
@@ -361,6 +356,9 @@ ServiceWorkerProviderHost* ServiceWorkerContextCore::GetProviderHost(
 void ServiceWorkerContextCore::RemoveProviderHost(
     int process_id, int provider_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // Precreated hosts are stored in the same map regardless of process.
+  if (ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id))
+    process_id = ChildProcessHost::kInvalidUniqueID;
   ProviderMap* map = GetProviderMapForProcess(process_id);
   DCHECK(map);
   map->Remove(provider_id);
@@ -371,6 +369,10 @@ void ServiceWorkerContextCore::RemoveAllProviderHostsForProcess(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (providers_->Lookup(process_id))
     providers_->Remove(process_id);
+  // This function is used to prevent <process_id, provider_id> collisions when
+  // a render process host is reused with the same process id. Don't bother
+  // removing the providers in this process with browser-assigned ids, which
+  // live in a different map, since they have unique ids.
 }
 
 std::unique_ptr<ServiceWorkerContextCore::ProviderHostIterator>

content/browser/service_worker/service_worker_context_core.h

@@ -70,6 +70,7 @@ class CONTENT_EXPORT ServiceWorkerContextCore
                               int64_t registration_id)>;
   using UnregistrationCallback =
       base::OnceCallback<void(blink::ServiceWorkerStatusCode status)>;
+  // TODO(falken): Change these to just use std::map.
   using ProviderMap = base::IDMap<std::unique_ptr<ServiceWorkerProviderHost>>;
   using ProcessToProviderMap = base::IDMap<std::unique_ptr<ProviderMap>>;
 
@@ -80,6 +81,8 @@ class CONTENT_EXPORT ServiceWorkerContextCore
   static const base::FilePath::CharType kServiceWorkerDirectory[];
 
   // Iterates over ServiceWorkerProviderHost objects in a ProcessToProviderMap.
+  // TODO(falken): This can just iterate over the simple map
+  // |providers_by_uuid_| for simplicity.
   class CONTENT_EXPORT ProviderHostIterator {
    public:
     ~ProviderHostIterator();
@@ -161,12 +164,12 @@ class CONTENT_EXPORT ServiceWorkerContextCore
   }
 
   // The context class owns the set of ProviderHosts.
+  //
+  // For browser-assigned provider ids, the |process_id| parameter is ignored,
+  // since they have unique ids.
   void AddProviderHost(
       std::unique_ptr<ServiceWorkerProviderHost> provider_host);
   ServiceWorkerProviderHost* GetProviderHost(int process_id, int provider_id);
-  std::unique_ptr<ServiceWorkerProviderHost> ReleaseProviderHost(
-      int process_id,
-      int provider_id);
   void RemoveProviderHost(int process_id, int provider_id);
   void RemoveAllProviderHostsForProcess(int process_id);
 
@@ -334,8 +337,13 @@ 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_;
+
+  // |providers_| owns the provider hosts. Hosts with browser-assigned provider
+  // ids (unique over all processes), are kept in the map for process id -1.
   std::unique_ptr<ProcessToProviderMap> providers_;
+  // |provider_by_uuid_| contains raw pointers to hosts owned by |providers_|.
   std::unique_ptr<ProviderByClientUUIDMap> provider_by_uuid_;
+
   std::unique_ptr<ServiceWorkerStorage> storage_;
   scoped_refptr<EmbeddedWorkerRegistry> embedded_worker_registry_;
   std::unique_ptr<ServiceWorkerJobCoordinator> job_coordinator_;

content/browser/service_worker/service_worker_dispatcher_host.cc

@@ -56,10 +56,6 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
   ServiceWorkerContextCore* context = context_wrapper_->context();
   if (!context)
     return;
-  if (context->GetProviderHost(render_process_id_, info->provider_id)) {
-    bindings_.ReportBadMessage("SWDH_PROVIDER_CREATED_DUPLICATE_ID");
-    return;
-  }
 
   // Provider hosts for navigations are precreated on the browser process with a
   // browser-assigned id. The renderer process calls OnProviderCreated once it
@@ -72,9 +68,8 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
     }
 
     // Retrieve the provider host pre-created for the navigation.
-    std::unique_ptr<ServiceWorkerProviderHost> provider_host =
-        context->ReleaseProviderHost(ChildProcessHost::kInvalidUniqueID,
-                                     info->provider_id);
+    ServiceWorkerProviderHost* provider_host = context->GetProviderHost(
+        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) {
@@ -84,9 +79,12 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
     }
 
     // Otherwise, complete initialization of the pre-created host.
+    if (provider_host->process_id() != ChildProcessHost::kInvalidUniqueID) {
+      bindings_.ReportBadMessage("SWDH_PRECREATED_PROVIDER_RESUED");
+      return;
+    }
     provider_host->CompleteNavigationInitialized(render_process_id_,
                                                  std::move(info));
-    context->AddProviderHost(std::move(provider_host));
     return;
   }
 
@@ -100,6 +98,10 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(
     return;
   }
 
+  if (context->GetProviderHost(render_process_id_, info->provider_id)) {
+    bindings_.ReportBadMessage("SWDH_PROVIDER_CREATED_DUPLICATE_ID");
+    return;
+  }
   context->AddProviderHost(ServiceWorkerProviderHost::Create(
       render_process_id_, std::move(info), context->AsWeakPtr()));
 }

content/browser/service_worker/service_worker_dispatcher_host_unittest.cc

@@ -162,70 +162,113 @@ class ServiceWorkerDispatcherHostTest : public testing::Test {
   std::vector<std::string> bad_messages_;
 };
 
-TEST_F(ServiceWorkerDispatcherHostTest, ProviderCreatedAndDestroyed) {
+TEST_F(ServiceWorkerDispatcherHostTest, ProviderCreatedForNavigation) {
   int process_id = helper_->mock_render_process_id();
-  std::unique_ptr<ServiceWorkerNavigationHandleCore> navigation_handle_core;
 
-  {
-    // Prepare the first navigation handle to create provider host.
-    const int kProviderId = -2;
-    navigation_handle_core =
+  // Prepare the navigation handle to create provider host.
+  std::unique_ptr<ServiceWorkerNavigationHandleCore> navigation_handle_core =
       CreateNavigationHandleCore(helper_->context_wrapper());
-    ASSERT_TRUE(navigation_handle_core);
   base::WeakPtr<ServiceWorkerProviderHost> host =
       ServiceWorkerProviderHost::PreCreateNavigationHost(
           context()->AsWeakPtr(), true /* are_ancestors_secure */,
           base::RepeatingCallback<WebContents*(void)>());
-    EXPECT_EQ(kProviderId, host->provider_id());
+  int provider_id = host->provider_id();
   mojom::ServiceWorkerProviderHostInfoPtr host_info =
-        CreateProviderHostInfoForWindow(kProviderId, 1 /* route_id */);
-    navigation_handle_core->DidPreCreateProviderHost(kProviderId);
+      CreateProviderHostInfoForWindow(provider_id, 1 /* route_id */);
+  EXPECT_TRUE(context()->GetProviderHost(ChildProcessHost::kInvalidUniqueID,
+                                         provider_id));
+  EXPECT_TRUE(context()->GetProviderHost(process_id, provider_id));
+  navigation_handle_core->DidPreCreateProviderHost(provider_id);
   RemoteProviderInfo remote_provider =
       SendProviderCreated(std::move(host_info));
-    EXPECT_TRUE(context()->GetProviderHost(process_id, kProviderId));
 
-    // Releasing the interface pointer destroys the counterpart.
+  // Releasing the interface pointer destroys the host.
   remote_provider.host_ptr.reset();
   base::RunLoop().RunUntilIdle();
-    EXPECT_FALSE(context()->GetProviderHost(process_id, kProviderId));
-  }
+  EXPECT_FALSE(context()->GetProviderHost(ChildProcessHost::kInvalidUniqueID,
+                                          provider_id));
+  EXPECT_FALSE(context()->GetProviderHost(process_id, provider_id));
+}
 
-  {
-    // Two with the same ID should be seen as a bad message.
-    const int kProviderId = 99;
-    RemoteProviderInfo remote_provider_1 = SendProviderCreated(
-        CreateProviderHostInfoForWindow(kProviderId, 1 /* route_id */));
+// Two with the same browser-assigned ID should be seen as a bad message.
+TEST_F(ServiceWorkerDispatcherHostTest, ReusedProviderCreatedForNavigation) {
+  int process_id = helper_->mock_render_process_id();
+
+  std::unique_ptr<ServiceWorkerNavigationHandleCore> navigation_handle_core =
+      CreateNavigationHandleCore(helper_->context_wrapper());
+  base::WeakPtr<ServiceWorkerProviderHost> host =
+      ServiceWorkerProviderHost::PreCreateNavigationHost(
+          context()->AsWeakPtr(), true /* are_ancestors_secure */,
+          base::RepeatingCallback<WebContents*(void)>());
+  int provider_id = host->provider_id();
+  EXPECT_EQ(provider_id, host->provider_id());
+  mojom::ServiceWorkerProviderHostInfoPtr host_info =
+      CreateProviderHostInfoForWindow(provider_id, 1 /* route_id */);
+  EXPECT_TRUE(context()->GetProviderHost(ChildProcessHost::kInvalidUniqueID,
+                                         provider_id));
+  EXPECT_TRUE(context()->GetProviderHost(process_id, provider_id));
+  navigation_handle_core->DidPreCreateProviderHost(provider_id);
+  RemoteProviderInfo remote_provider =
+      SendProviderCreated(std::move(host_info));
+
+  host_info = CreateProviderHostInfoForWindow(provider_id, 2 /* route_id */);
+  RemoteProviderInfo remote_provider2 =
+      SendProviderCreated(std::move(host_info));
 
-    EXPECT_TRUE(bad_messages_.empty());
-    RemoteProviderInfo remote_provider_2 = SendProviderCreated(
-        CreateProviderHostInfoForWindow(kProviderId, 1 /* route_id */));
   ASSERT_EQ(1u, bad_messages_.size());
-    EXPECT_EQ("SWDH_PROVIDER_CREATED_DUPLICATE_ID", bad_messages_[0]);
-  }
+  EXPECT_EQ("SWDH_PRECREATED_PROVIDER_RESUED", bad_messages_[0]);
+
+  // Releasing the interface pointer destroys the host.
+  remote_provider.host_ptr.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(context()->GetProviderHost(ChildProcessHost::kInvalidUniqueID,
+                                          provider_id));
+  EXPECT_FALSE(context()->GetProviderHost(process_id, provider_id));
+}
+
+TEST_F(ServiceWorkerDispatcherHostTest,
+       RendererDiesWithProviderCreatedForNavigation) {
+  int process_id = helper_->mock_render_process_id();
 
-  {
-    // Prepare another navigation handle to create another provider host.
-    const int kProviderId = -3;
-    navigation_handle_core =
+  // Prepare a navigation handle to create a provider host.
+  std::unique_ptr<ServiceWorkerNavigationHandleCore> navigation_handle_core =
       CreateNavigationHandleCore(helper_->context_wrapper());
   base::WeakPtr<ServiceWorkerProviderHost> host =
       ServiceWorkerProviderHost::PreCreateNavigationHost(
           context()->AsWeakPtr(), true /* are_ancestors_secure */,
           base::RepeatingCallback<WebContents*(void)>());
-    EXPECT_EQ(kProviderId, host->provider_id());
+  int provider_id = host->provider_id();
   mojom::ServiceWorkerProviderHostInfoPtr host_info =
-        CreateProviderHostInfoForWindow(kProviderId, 2 /* route_id */);
-    navigation_handle_core->DidPreCreateProviderHost(kProviderId);
+      CreateProviderHostInfoForWindow(provider_id, 2 /* route_id */);
+  navigation_handle_core->DidPreCreateProviderHost(provider_id);
   RemoteProviderInfo remote_provider =
       SendProviderCreated(std::move(host_info));
-    EXPECT_TRUE(context()->GetProviderHost(process_id, kProviderId));
+  EXPECT_TRUE(context()->GetProviderHost(process_id, provider_id));
 
   // Simulate that the corresponding renderer process died.
   dispatcher_host_->RenderProcessExited(nullptr /* host */,
                                         ChildProcessTerminationInfo());
   base::RunLoop().RunUntilIdle();
-    EXPECT_FALSE(context()->GetProviderHost(process_id, kProviderId));
-  }
+  // The host still exists but is removed when the Mojo connection is
+  // destroyed.
+  EXPECT_TRUE(context()->GetProviderHost(process_id, provider_id));
+  // Releasing the interface pointer destroys the host.
+  remote_provider.host_ptr.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(context()->GetProviderHost(process_id, provider_id));
+}
+
+// Two with the same renderer-assigned ID should be seen as a bad message.
+TEST_F(ServiceWorkerDispatcherHostTest, DuplicateProvider) {
+  const int kProviderId = 99;
+  RemoteProviderInfo remote_provider_1 = SendProviderCreated(
+      CreateProviderHostInfoForWindow(kProviderId, 1 /* route_id */));
+
+  EXPECT_TRUE(bad_messages_.empty());
+  RemoteProviderInfo remote_provider_2 = SendProviderCreated(
+      CreateProviderHostInfoForWindow(kProviderId, 1 /* route_id */));
+  ASSERT_EQ(1u, bad_messages_.size());
+  EXPECT_EQ("SWDH_PROVIDER_CREATED_DUPLICATE_ID", bad_messages_[0]);
 }
 
 TEST_F(ServiceWorkerDispatcherHostTest, CleanupOnRendererCrash) {

content/browser/service_worker/service_worker_navigation_handle_core.cc

@@ -31,13 +31,21 @@ ServiceWorkerNavigationHandleCore::~ServiceWorkerNavigationHandleCore() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (provider_id_ == kInvalidServiceWorkerProviderId)
     return;
-  // Remove the provider host if it was never completed (navigation failed).
   ServiceWorkerContextCore* context = context_wrapper_->context();
-  if (!context || !context->GetProviderHost(ChildProcessHost::kInvalidUniqueID,
-                                            provider_id_)) {
+  if (!context)
+    return;
+  ServiceWorkerProviderHost* host = context->GetProviderHost(
+      ChildProcessHost::kInvalidUniqueID, provider_id_);
+  if (!host)
     return;
-  }
-  context->RemoveProviderHost(ChildProcessHost::kInvalidUniqueID, provider_id_);
+  // Remove the provider host if it was never completed (navigation failed).
+  // TODO(falken): ServiceWorkerNavigationHandleCore should just own a Mojo
+  // pointer tied to the lifetime of ServiceWorkerProviderHost, and send the
+  // Mojo pointer to the renderer on navigation commit. If the handle core dies
+  // before that, the provider host would be destroyed by Mojo connection error.
+  if (!host->is_execution_ready())
+    context->RemoveProviderHost(ChildProcessHost::kInvalidUniqueID,
+                                provider_id_);
 }
 
 void ServiceWorkerNavigationHandleCore::DidPreCreateProviderHost(

content/browser/service_worker/service_worker_provider_host_unittest.cc

@@ -179,12 +179,8 @@ class ServiceWorkerProviderHostTest : public testing::TestWithParam<bool> {
 
     // In production code, the OnProviderCreated IPC is received which
     // does this.
-    std::unique_ptr<ServiceWorkerProviderHost> owned_host =
-        helper_->context()->ReleaseProviderHost(host->process_id(),
-                                                host->provider_id());
     host->CompleteNavigationInitialized(helper_->mock_render_process_id(),
                                         std::move(info));
-    helper_->context()->AddProviderHost(std::move(owned_host));
   }
 
   blink::mojom::ServiceWorkerErrorType Register(
@@ -312,15 +308,10 @@ class ServiceWorkerProviderHostTest : public testing::TestWithParam<bool> {
         CreateProviderHostInfoForWindow(host->provider_id(), 1 /* route_id */);
     remote_endpoint->BindWithProviderHostInfo(&info);
 
-    std::unique_ptr<ServiceWorkerProviderHost> owned_host =
-        helper_->context()->ReleaseProviderHost(host->process_id(),
-                                                host->provider_id());
     host->CompleteNavigationInitialized(helper_->mock_render_process_id(),
                                         std::move(info));
     host->SetDocumentUrl(document_url);
     host->SetTopmostFrameUrl(topmost_frame_url);
-    helper_->context()->AddProviderHost(std::move(owned_host));
-
     return host.get();
   }