Enable spare RenderProcessHost to be preinitialized.

content/browser/renderer_host/render_process_host_browsertest.cc

Index: content/browser/renderer_host/render_process_host_browsertest.cc
diff --git a/content/browser/renderer_host/render_process_host_browsertest.cc b/content/browser/renderer_host/render_process_host_browsertest.cc
index 866e829d1b74c93dad22fc5b77ee71e84cf19598..78f95b58d58a8ff471e1c9725d760a3c945ec6e3 100644
--- a/content/browser/renderer_host/render_process_host_browsertest.cc
+++ b/content/browser/renderer_host/render_process_host_browsertest.cc
@@ -17,6 +17,9 @@
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_service.mojom.h"
 #include "content/shell/browser/shell.h"
+#include "content/shell/browser/shell_browser_context.h"
+#include "content/shell/browser/shell_browser_main_parts.h"
+#include "content/shell/browser/shell_content_browser_client.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/media_switches.h"
 #include "media/base/test_data_util.h"
@@ -130,6 +133,116 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
   EXPECT_EQ(2, RenderProcessHostCount());
 }
 
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, SpareRenderProcessHostTaken) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  RenderProcessHost::WarmupSpareRenderProcessHost(
+      ShellContentBrowserClient::Get()->browser_context());
+  RenderProcessHost* spare_renderer =
+      RenderProcessHostImpl::GetSpareRenderProcessHostForTesting();
+  EXPECT_NE(nullptr, spare_renderer);
+
+  GURL test_url = embedded_test_server()->GetURL("/simple_page.html");
+  Shell* window = CreateBrowser();
+  NavigateToURL(window, test_url);
+
+  EXPECT_EQ(spare_renderer,
+            window->web_contents()->GetMainFrame()->GetProcess());
+
+  // The spare render process host should no longer be available.
+  EXPECT_EQ(nullptr,
+            RenderProcessHostImpl::GetSpareRenderProcessHostForTesting());
+}
+
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, SpareRenderProcessHostNotTaken) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  RenderProcessHost::WarmupSpareRenderProcessHost(
+      ShellContentBrowserClient::Get()->off_the_record_browser_context());
+  RenderProcessHost* spare_renderer =
+      RenderProcessHostImpl::GetSpareRenderProcessHostForTesting();
+  GURL test_url = embedded_test_server()->GetURL("/simple_page.html");
+  Shell* window = CreateBrowser();
+  NavigateToURL(window, test_url);
+
+  // There should have been another process created for the navigation.
+  EXPECT_NE(spare_renderer,
+            window->web_contents()->GetMainFrame()->GetProcess());
+
+  // The spare RenderProcessHost should have been cleaned up. Note this
+  // behavior is identical to what would have happened if the RenderProcessHost
+  // were taken.
+  EXPECT_EQ(nullptr,
+            RenderProcessHostImpl::GetSpareRenderProcessHostForTesting());
+}
+
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, SpareRenderProcessHostKilled) {
+  RenderProcessHost::WarmupSpareRenderProcessHost(
+      ShellContentBrowserClient::Get()->browser_context());
+
+  RenderProcessHost* spare_renderer =
+      RenderProcessHostImpl::GetSpareRenderProcessHostForTesting();
+  mojom::TestServicePtr service;
+  ASSERT_NE(nullptr, spare_renderer);
+  BindInterface(spare_renderer, &service);
+
+  base::RunLoop run_loop;
+  set_process_exit_callback(run_loop.QuitClosure());
+  spare_renderer->AddObserver(this);  // For process_exit_callback.
+
+  // Should reply with a bad message and cause process death.
+  service->DoSomething(base::Bind(&base::DoNothing));
+  run_loop.Run();
+
+  // The spare RenderProcessHost should disappear when its process dies.
+  EXPECT_EQ(nullptr,
+            RenderProcessHostImpl::GetSpareRenderProcessHostForTesting());
+}
+
+// Test that the spare renderer works correctly when the limit on the maximum
+// number of processes is small.
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
+                       SpareRendererSurpressedMaxProcesses) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  RenderProcessHost::SetMaxRendererProcessCount(1);
+
+  // A process is created with shell startup, so with a maximum of one renderer
+  // process the spare RPH should not be created.
+  RenderProcessHost::WarmupSpareRenderProcessHost(
+      ShellContentBrowserClient::Get()->browser_context());
+  EXPECT_EQ(nullptr,
+            RenderProcessHostImpl::GetSpareRenderProcessHostForTesting());
+
+  // A spare RPH should be created with a max of 2 renderer process.

[Charlie Reis, 2017/06/29 21:09:16]

nit: s/process/processes/

[mattcary, 2017/06/30 12:37:56]

Done.

+  RenderProcessHost::SetMaxRendererProcessCount(2);
+  RenderProcessHost::WarmupSpareRenderProcessHost(
+      ShellContentBrowserClient::Get()->browser_context());
+  RenderProcessHost* spare_renderer =
+      RenderProcessHostImpl::GetSpareRenderProcessHostForTesting();
+  EXPECT_NE(nullptr, spare_renderer);
+
+  // Navigation will use the spare renderer half the time, choosing the spare
+  // renderer or the existing render for the new navigation at random.

[Charlie Reis, 2017/06/29 21:09:16]

nit: s/render/renderer/

[mattcary, 2017/06/30 12:37:55]

Done.

+  GURL test_url = embedded_test_server()->GetURL("/simple_page.html");
+  NavigateToURL(CreateBrowser(), test_url);
+
+  // Revert to the default process limit.
+  RenderProcessHost::SetMaxRendererProcessCount(0);
+
+  // If the spare renderer wasn't picked up for the naviation above, perform

[Charlie Reis, 2017/06/29 21:09:16]

nit: navigation

[mattcary, 2017/06/30 12:37:56]

Done.

+  // another navigation in a new shell, without a process limit, and confirm
+  // that the spare renderer is used correctly. Note this branch is entered
+  // randomly, depending on the choice of renderer used above, so the check
+  // below will only fail in a flaky manner (but will not flake at all if
+  // everything is working correctly, of course!).
+  if (RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()) {
+    NavigateToURL(CreateBrowser(), test_url);
+    EXPECT_EQ(nullptr,
+              RenderProcessHostImpl::GetSpareRenderProcessHostForTesting());

[Charlie Reis, 2017/06/29 21:09:16]

This doesn't look right to me, and it's never failing for me locally without the
fix.  Suppose we left the spare renderer registered as a spare when we used it
in the navigation on line 228.  Now that we've removed the process limit on line
231, the navigation on line 240 will definitely use the spare (unexpectedly
sharing with the earlier navigation), and this check will never fail.

It seems like we want to check that the first navigation's process is not equal
to the spare process (either because we picked the spare RPH and cleared it, or
because we didn't pick it).  We also want to check that the spare RPH doesn't
have anything in it if it still exists.  In other words, replace lines 228-243
with:

  Shell* new_browser = CreateBrowser();
  NavigateToURL(new_browser, test_url);
  EXPECT_NE(RenderProcessHostImpl::GetSpareRenderProcessHostForTesting(),
            new_browser->web_contents()->GetMainFrame()->GetProcess());

  if (RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()) {
    EXPECT_EQ(0, RenderProcessHostImpl::GetSpareRenderProcessHostForTesting()
                     ->VisibleWidgetCount());
  }

Beyond that, I'd like to avoid having this test be non-deterministic.  (Flaky
tests tend to get disabled when they start failing, not fixed.)  I think we can
pull that off using a TestContentBrowserClient that only returns true from
IsSuitableHost for the spare RPH.  That should force the outcome we want to test
and make it deterministic.

You can see an example of using a TestContentBrowserClient in a browser test in
ServiceWorkerVersionBrowserTest.FetchWithSaveData.

[mattcary, 2017/06/30 12:37:55]

Right, this is what I meant about not being able to tell how a spare renderer
was consumed. If I remove the fix in GetExistingProcessHost, the spare renderer
can get reused for the second navigation in this if statement. If that would
cause things to crash, this test would pick that up. Unfortunately, currently if
an RPH is reused nothing crashes.

With your suggestion for checking that it's unused, that's no longer the case. I
also like the idea of checking the widget count rather than just navigating and
seeing if anything breaks.
That seemed to be easy enough. With this and the other changes, if I remove the
DropSpare in GetExistingProcessHost, the test fails deterministically. Hooray!

+  }
+}
+
 class ShellCloser : public RenderProcessHostObserver {
  public:
   ShellCloser(Shell* shell, std::string* logging_string)