Enable spare RenderProcessHost to be preinitialized.

content/browser/renderer_host/render_process_host_impl.h

 // Copyright (c) 2012 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.
 
 #ifndef CONTENT_BROWSER_RENDERER_HOST_RENDER_PROCESS_HOST_IMPL_H_
 #define CONTENT_BROWSER_RENDERER_HOST_RENDER_PROCESS_HOST_IMPL_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 96 matching lines @@
 // because all the IPCs from the RenderViews (renderer) will only ever be able
 // to access the partition they are assigned to.
 class CONTENT_EXPORT RenderProcessHostImpl
     : public RenderProcessHost,
       public ChildProcessLauncher::Client,
       public ui::GpuSwitchingObserver,
       public NON_EXPORTED_BASE(mojom::RouteProvider),
       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).
   bool Init() override;
   void EnableSendQueue() override;
   int GetNextRoutingID() override;
   void AddRoute(int32_t routing_id, IPC::Listener* listener) override;
   void RemoveRoute(int32_t routing_id) override;
   void AddObserver(RenderProcessHostObserver* observer) override;
   void RemoveObserver(RenderProcessHostObserver* observer) override;
@@ skipping 200 matching lines @@
   // RenderProcessHosts.
   static void AddExpectedNavigationToSite(
       BrowserContext* browser_context,
       RenderProcessHost* render_process_host,
       const GURL& site_url);
   static void RemoveExpectedNavigationToSite(
       BrowserContext* browser_context,
       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_;
 
   // True if fast shutdown has been performed on this RPH.
   bool fast_shutdown_started_;
 
   // True if we've posted a DeleteTask and will be deleted soon.
   bool deleting_soon_;
 
 #ifndef NDEBUG
   // True if this object has deleted itself.
   bool is_self_deleted_;
 #endif
 
   // The count of currently swapped out but pending RenderViews.  We have
   // started to swap these in, so the renderer process should not exit if
   // this count is non-zero.
   int32_t pending_views_;
 
  private:
   friend class ChildProcessLauncherBrowserTest_ChildSpawnFail_Test;
   friend class VisitRelayingRenderProcessHost;
   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();
 
   // Resets |channel_|, removing it from the attachment broker if necessary.
   // Always call this in lieu of directly resetting |channel_|.
   void ResetChannelProxy();
 
   // Creates and adds the IO thread message filters.
   void CreateMessageFilters();
@@ skipping 337 matching lines @@
   FrameSinkProviderImpl frame_sink_provider_;
 
   base::WeakPtrFactory<RenderProcessHostImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderProcessHostImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_RENDER_PROCESS_HOST_IMPL_H_