Let service worker and navigations use the same process from Android's NTP.

Android's New Tab Page has an empty site URL by design to let navigations
from the NTP use the same process. The site URL is assigned after the
navigation is committed. However, this doesn't interact well with
SW. SW is created in a process suitable for the service worker's script
URL, and the navigation stays in the NTP process, so the SW is
out-of-process which has a performance and memory cost.

With this CL, when the SW process is chosen, it takes a process that
hasn't yet been assigned to a SiteInstance. The idea is to choose the
Android NTP process.

Bug: 1012143
Change-Id: I423de186982a11c68e4c2bec7b5ca56b3245f62c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849473
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707274}

content/browser/renderer_host/render_process_host_impl.cc

@@ -3932,6 +3932,41 @@ RenderProcessHost* RenderProcessHostImpl::GetExistingProcessHost(
   return nullptr;
 }
 
+// static
+RenderProcessHost* RenderProcessHostImpl::GetUnusedProcessHostForServiceWorker(
+    SiteInstanceImpl* site_instance) {
+  DCHECK(site_instance->is_for_service_worker());
+  if (site_instance->process_reuse_policy() !=
+      SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE) {
+    return nullptr;
+  }
+
+  auto& spare_process_manager = SpareRenderProcessHostManager::GetInstance();
+  iterator iter(AllHostsIterator());
+  while (!iter.IsAtEnd()) {
+    auto* host = iter.GetCurrentValue();
+    // This function tries to choose the process that will be chosen by a
+    // navigation that will use the service worker that is being started. Prefer
+    // to not take the spare process host, since if the navigation is out of the
+    // New Tab Page on Android, it will be using the existing NTP process
+    // instead of the spare process. If this function doesn't find a suitable
+    // process, the spare can still be chosen when
+    // MaybeTakeSpareRenderProcessHost() is called later.
+    bool is_spare = (host == spare_process_manager.spare_render_process_host());
+
+    if (!is_spare && iter.GetCurrentValue()->MayReuseHost() &&
+        iter.GetCurrentValue()->IsUnused() &&
+        RenderProcessHostImpl::IsSuitableHost(
+            iter.GetCurrentValue(), site_instance->GetBrowserContext(),
+            site_instance->GetIsolationContext(), site_instance->GetSiteURL(),
+            site_instance->lock_url())) {
+      return host;
+    }
+    iter.Advance();
+  }
+  return nullptr;
+}
+
 // static
 bool RenderProcessHost::ShouldUseProcessPerSite(BrowserContext* browser_context,
                                                 const GURL& url) {
@@ -4064,6 +4099,13 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
                features::kProcessSharingWithStrictSiteInstances));
   }
 
+  // If a process hasn't been selected yet, and the site instance is for a
+  // service worker, try to use an unused process host. One might have been
+  // created for a navigation and this will let the navigation and the service
+  // worker share the same process.
+  if (!render_process_host && is_unmatched_service_worker)
+    render_process_host = GetUnusedProcessHostForServiceWorker(site_instance);
+
   // See if the spare RenderProcessHost can be used.
   auto& spare_process_manager = SpareRenderProcessHostManager::GetInstance();
   bool spare_was_taken = false;

content/browser/renderer_host/render_process_host_impl.h

@@ -715,6 +715,22 @@ class CONTENT_EXPORT RenderProcessHostImpl
   FRIEND_TEST_ALL_PREFIXES(RenderProcessHostUnitTest,
                            GuestsAreNotSuitableHosts);
 
+  // Returns an existing RenderProcessHost that has not yet been used and is
+  // suitable for the given |site_instance|, or null if no such process host
+  // exists.
+  //
+  // This function is used when finding a process for a service worker. The
+  // idea is to choose the process that will be chosen by a navigation that will
+  // use the service worker. While navigations typically try to choose the
+  // process with the relevant service worker (using
+  // UnmatchedServiceWorkerProcessTracker), navigations out of the Android New
+  // Tab Page use a SiteInstance with an empty URL by design in order to choose
+  // the NTP process, and do not go through the typical matching algorithm. The
+  // goal of this function is to return the NTP process so the service worker
+  // can also use it.
+  static RenderProcessHost* GetUnusedProcessHostForServiceWorker(
+      SiteInstanceImpl* site_instance);
+
   // Returns a RenderProcessHost that is rendering a URL corresponding to
   // |site_instance| in one of its frames, or that is expecting a navigation to
   // that SiteInstance.

content/browser/service_worker/service_worker_process_browsertest.cc

+// Copyright 2019 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 "base/command_line.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/storage_partition.h"
+#include "content/public/common/content_client.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "content/test/test_content_browser_client.h"
+#include "net/dns/mock_host_resolver.h"
+
+// This file has tests involving render process selection for service workers.
+
+namespace content {
+
+class ServiceWorkerProcessBrowserTest
+    : public ContentBrowserTest,
+      public ::testing::WithParamInterface<bool> {
+ public:
+  ServiceWorkerProcessBrowserTest() = default;
+  ~ServiceWorkerProcessBrowserTest() override = default;
+
+  ServiceWorkerProcessBrowserTest(const ServiceWorkerProcessBrowserTest&) =
+      delete;
+  ServiceWorkerProcessBrowserTest& operator=(
+      const ServiceWorkerProcessBrowserTest&) = delete;
+
+  void SetUpOnMainThread() override {
+    // Support multiple sites on the test server.
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->Start());
+
+    StoragePartition* partition = BrowserContext::GetDefaultStoragePartition(
+        shell()->web_contents()->GetBrowserContext());
+    wrapper_ = static_cast<ServiceWorkerContextWrapper*>(
+        partition->GetServiceWorkerContext());
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    if (SitePerProcess())
+      command_line->AppendSwitch(switches::kSitePerProcess);
+  }
+
+ protected:
+  bool SitePerProcess() const { return GetParam(); }
+
+  // Registers a service worker and then tears down the process it used, for a
+  // clean slate going forward.
+  void RegisterServiceWorker() {
+    // Load a page that registers a service worker.
+    Shell* start_shell = CreateBrowser();
+    ASSERT_TRUE(NavigateToURL(
+        start_shell, embedded_test_server()->GetURL(
+                         "/service_worker/create_service_worker.html")));
+    ASSERT_EQ("DONE",
+              EvalJs(start_shell, "register('fetch_event_pass_through.js');"));
+
+    auto* host = RenderProcessHost::FromID(GetServiceWorkerProcessId());
+    ASSERT_TRUE(host);
+    RenderProcessHostWatcher exit_watcher(
+        host, RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+
+    // Tear down the page.
+    start_shell->Close();
+
+    // Stop the service worker. The process should exit.
+    base::RunLoop loop;
+    wrapper()->StopAllServiceWorkers(loop.QuitClosure());
+    loop.Run();
+    exit_watcher.Wait();
+  }
+
+  // Returns the number of running service workers.
+  size_t GetRunningServiceWorkerCount() {
+    return wrapper()->GetRunningServiceWorkerInfos().size();
+  }
+
+  // Returns the process id of the running service worker. There must be exactly
+  // one service worker running.
+  int GetServiceWorkerProcessId() {
+    const base::flat_map<int64_t, ServiceWorkerRunningInfo>& infos =
+        wrapper()->GetRunningServiceWorkerInfos();
+    DCHECK_EQ(infos.size(), 1u);
+    const ServiceWorkerRunningInfo& info = infos.begin()->second;
+    return info.render_process_id;
+  }
+
+  ServiceWorkerContextWrapper* wrapper() { return wrapper_.get(); }
+
+  WebContentsImpl* web_contents() {
+    return static_cast<WebContentsImpl*>(shell()->web_contents());
+  }
+
+  RenderFrameHostImpl* current_frame_host() {
+    return web_contents()->GetFrameTree()->root()->current_frame_host();
+  }
+
+ private:
+  scoped_refptr<ServiceWorkerContextWrapper> wrapper_;
+};
+
+// Tests that a service worker started due to a navigation shares the same
+// process as the navigation.
+IN_PROC_BROWSER_TEST_P(ServiceWorkerProcessBrowserTest,
+                       ServiceWorkerAndPageShareProcess) {
+  // Register the service worker.
+  RegisterServiceWorker();
+
+  // Navigate to a page in the service worker's scope.
+  ASSERT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("/service_worker/empty.html")));
+
+  // The page and service worker should be in the same process.
+  int page_process_id = current_frame_host()->GetProcess()->GetID();
+  EXPECT_NE(page_process_id, ChildProcessHost::kInvalidUniqueID);
+  ASSERT_EQ(GetRunningServiceWorkerCount(), 1u);
+  int worker_process_id = GetServiceWorkerProcessId();
+  EXPECT_EQ(page_process_id, worker_process_id);
+}
+
+// ContentBrowserClient that skips assigning a site URL for a given URL.
+class DontAssignSiteContentBrowserClient : public TestContentBrowserClient {
+ public:
+  // Any visit to |url_to_skip| will not cause the site to be assigned to the
+  // SiteInstance.
+  explicit DontAssignSiteContentBrowserClient(const GURL& url_to_skip)
+      : url_to_skip_(url_to_skip) {}
+
+  DontAssignSiteContentBrowserClient(
+      const DontAssignSiteContentBrowserClient&) = delete;
+  DontAssignSiteContentBrowserClient& operator=(
+      const DontAssignSiteContentBrowserClient&) = delete;
+
+  bool ShouldAssignSiteForURL(const GURL& url) override {
+    return url == url_to_skip_;
+  }
+
+ private:
+  GURL url_to_skip_;
+};
+
+// Tests that a service worker and navigation share the same process in the
+// special case where the service worker starts before the navigation starts,
+// and the navigation transitions out of a page with no site URL. This special
+// case happens in real life when doing a search from the omnibox while on the
+// Android native NTP page: the service worker starts first due to the
+// navigation hint from the omnibox, and the native page has no site URL. See
+// https://crbug.com/1012143.
+IN_PROC_BROWSER_TEST_P(
+    ServiceWorkerProcessBrowserTest,
+    ServiceWorkerAndPageShareProcess_NavigateFromUnassignedSiteInstance) {
+  // Set up a page URL that will have no site URL.
+  GURL empty_site = embedded_test_server()->GetURL("a.com", "/title1.html");
+  DontAssignSiteContentBrowserClient content_browser_client(empty_site);
+  ContentBrowserClient* old_client =
+      SetBrowserClientForTesting(&content_browser_client);
+
+  // Register the service worker.
+  RegisterServiceWorker();
+
+  // Navigate to the empty site instance page. Subsequent navigations from
+  // this page will prefer to use the same process by default.
+  ASSERT_TRUE(NavigateToURL(shell(), empty_site));
+
+  // Start the service worker. It will start in a new process.
+  base::RunLoop loop;
+  GURL scope = embedded_test_server()->GetURL("/service_worker/");
+  wrapper()->StartWorkerForScope(
+      scope,
+      base::BindLambdaForTesting([&loop](int64_t version_id, int process_id,
+                                         int thread_id) { loop.Quit(); }),
+      base::BindLambdaForTesting([&loop]() {
+        ASSERT_FALSE(true) << "start worker failed";
+        loop.Quit();
+      }));
+  loop.Run();
+
+  // Navigate to a page in the service worker's scope.
+  ASSERT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("/service_worker/empty.html")));
+
+  // The page and service worker should be in the same process.
+  ASSERT_EQ(GetRunningServiceWorkerCount(), 1u);
+  int page_process_id = current_frame_host()->GetProcess()->GetID();
+  EXPECT_NE(page_process_id, ChildProcessHost::kInvalidUniqueID);
+  EXPECT_EQ(page_process_id, GetServiceWorkerProcessId());
+
+  SetBrowserClientForTesting(old_client);
+}
+
+// Toggle Site Isolation.
+INSTANTIATE_TEST_SUITE_P(, ServiceWorkerProcessBrowserTest, testing::Bool());
+
+}  // namespace content

content/test/BUILD.gn

@@ -1031,6 +1031,7 @@ test("content_browsertests") {
     "../browser/service_worker/service_worker_clients_api_browsertest.cc",
     "../browser/service_worker/service_worker_file_upload_browsertest.cc",
     "../browser/service_worker/service_worker_no_best_effort_tasks_browsertest.cc",
+    "../browser/service_worker/service_worker_process_browsertest.cc",
     "../browser/session_history_browsertest.cc",
     "../browser/shape_detection/shape_detection_browsertest.cc",
     "../browser/site_per_process_browsertest.cc",