Enable spare RenderProcessHost to be preinitialized.

content/browser/renderer_host/render_process_host_impl.h

Index: content/browser/renderer_host/render_process_host_impl.h
diff --git a/content/browser/renderer_host/render_process_host_impl.h b/content/browser/renderer_host/render_process_host_impl.h
index 96e664d759ddfc18147963e11f0da001cd67b3e8..f77e17d0c642a59a36d4f3c1d2a8bdf953aa2583 100644
--- a/content/browser/renderer_host/render_process_host_impl.h
+++ b/content/browser/renderer_host/render_process_host_impl.h
@@ -114,9 +114,20 @@ class CONTENT_EXPORT RenderProcessHostImpl
       public NON_EXPORTED_BASE(mojom::AssociatedInterfaceProvider),
       public NON_EXPORTED_BASE(mojom::RendererHost) {
  public:
-  RenderProcessHostImpl(BrowserContext* browser_context,
-                        StoragePartitionImpl* storage_partition_impl,
-                        bool is_for_guests_only);
+  // Use the spare RenderProcessHost if it exists, or create a new one. This
+  // should be the usual way to get a new RenderProcessHost.
+  static RenderProcessHost* CreateOrUseSpareRenderProcessHost(
+      BrowserContext* browser_context,
+      StoragePartitionImpl* storage_partition_impl,
+      bool is_for_guests_only);
+
+  // Create a new RenderProcessHost. In most cases
+  // CreateOrUseSpareRenderProcessHost, above, should be used instead.
+  static RenderProcessHost* CreateRenderProcessHost(
+      BrowserContext* browser_context,
+      StoragePartitionImpl* storage_partition_impl,
+      bool is_for_guests_only);
+
   ~RenderProcessHostImpl() override;
 
   // RenderProcessHost implementation (public portion).
@@ -337,6 +348,16 @@ class CONTENT_EXPORT RenderProcessHostImpl
       RenderProcessHost* render_process_host,
       const GURL& site_url);
 
+  // Return the spare RenderProcessHost, if it exists. There is at most one
+  // globally-used spare RenderProcessHost at any time.
+  static RenderProcessHost* GetSpareRenderProcessHostForTesting();
+
+  // Return true if the host has not been used. This is stronger than IsUnused()
+  // in that it checks if this RPH has ever been used to render at all, rather
+  // than just no being suitable to host a URL that requires a dedicated
+  // process.
+  bool HostHasNotBeenUsed();

[Charlie Reis, 2017/06/30 21:42:16]

Hmm, it's unfortunate to have this name be effectively the same as IsUnused()
but have a different meaning.

I suppose we can get by for the moment so that you can land this, but we should
decide whether one should be renamed or if this definition can work for both use
cases.  At least include a TODO(creis,alexmos) to rename or combine with
IsUnused.

alexmos@: Is there a problem with using this definition in your IsSuitableHost
call?  It's basically IsUnused() && listeners_.IsEmpty() && GetWorkerRefCount()
== 0 && pending_views_ == 0.  (I'm guessing that might be a problem, but there's
a chance it could be better, e.g., if we have a race where both a web page and a
page requiring a dedicated process decide to use the same process before either
one commits?)

[alexmos, 2017/07/01 00:47:17]

I'd need to check this more carefully, but I'm guessing the listeners_.IsEmpty()
might be a problem.  The main use case for IsUnused() was avoiding an
unnecessary process swap when a just-created blank tab with a site-less
SiteInstance was navigated to a site that requires a dedicated process.  It
should be fine to reuse the initially created process in that case, but with
HostHasNotBeenUsed, the RFH/RVH might be created and added to listeners_ before
IsSuitableHost() checks IsUnused().

Good catch about the race.  A page requiring a dedicated process should use a
SiteInstance with its site assigned, causing IsUnused to become false right
after GetProcess(), but there could still be a pending navigation for a web site
with a lazily-assigned site URL that had picked that process earlier but hasn't
committed yet.  I've just filed
https://bugs.chromium.org/p/chromium/issues/detail?id=738634 with steps that
actually seem to repro this.

I'm fine with adding a TODO for now and revisiting it as part of the bug above.

[mattcary, 2017/07/03 08:03:02]

Done. I assigned the TODO to you, alex, to be consistent with the bug, but I'm
happy to help with the follow-up (to the best of my ability...)

+
  protected:
   // A proxy for our IPC::Channel that lives on the IO thread.
   std::unique_ptr<IPC::ChannelProxy> channel_;
@@ -363,6 +384,12 @@ class CONTENT_EXPORT RenderProcessHostImpl
   class ConnectionFilterController;
   class ConnectionFilterImpl;
 
+  // Use CreateRenderProcessHost() instead of calling this constructor
+  // directly.
+  RenderProcessHostImpl(BrowserContext* browser_context,
+                        StoragePartitionImpl* storage_partition_impl,
+                        bool is_for_guests_only);
+
   // Initializes a new IPC::ChannelProxy in |channel_|, which will be connected
   // to the next child process launched for this host, if any.
   void InitializeChannelProxy();