Implement ProcessReusePolicy for SiteInstances

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 5a18d331b35d3e7c473a174fc3d3185d0686fb6a..0e89ed8b63c844b8faa9b6fc874639c208b4e026 100644
--- a/content/browser/renderer_host/render_process_host_impl.h
+++ b/content/browser/renderer_host/render_process_host_impl.h
@@ -71,10 +71,12 @@ class PermissionServiceContext;
 class PeerConnectionTrackerHost;
 class PushMessagingManager;
 class RenderFrameMessageFilter;
+class RenderProcessHostFactory;
 class RenderWidgetHelper;
 class RenderWidgetHost;
 class RenderWidgetHostImpl;
 class ResourceMessageFilter;
+class SiteInstanceImpl;
 class StoragePartition;
 class StoragePartitionImpl;
 
@@ -256,6 +258,31 @@ class CONTENT_EXPORT RenderProcessHostImpl
       RenderProcessHost* process,
       const GURL& url);
 
+  // The policy to apply when selecting a RenderProcessHost for a site.

[Charlie Reis, 2017/05/03 23:28:00]

I'm unclear if this is just about process-per-site style reuse or if we'll
extend it for all the types of reuse in
https://docs.google.com/a/chromium.org/document/d/1UrrmlcqwM5qIpqBM3ud3gYY5uG....

For example, normal SiteInstances don't proactively reuse processes, but when
over the limit, they reuse randomly, not based on site.  In contrast,
process-per-site and ServiceWorkers proactively join a process that already
contains the site.

The "for a site" part of this comment seems to imply it might be specifically
for the cases where affinity is based on site, but then again, it's handling
TDI's default subframe process already.  Maybe we should rephrase the comment
about how much we expect this will cover in the future?

[clamy, 2017/05/04 15:56:19]

I have rephrased the comment.

+  enum class ProcessReusePolicy {

[Charlie Reis, 2017/05/03 23:28:00]

It's a bit odd that this enum is not used in any APIs here.  Should we declare
it in SiteInstance(Impl?) instead?

[clamy, 2017/05/04 15:56:19]

Done.

+    // Do not attempt to reuse an existing RenderProcessHost.
+    DONT,

[Charlie Reis, 2017/05/03 23:28:00]

Let's clarify if this really means "never reuse, even when over the process
limit," since at first I thought it might mean "don't proactively reuse, unless
we're over the process limit" (until I saw DEFAULT).

Is there a case we know we need this for?  I see that it's easy to implement,
but I think there's still some open questions about whether it's needed.  For
example, even a site that requires a dedicated process could still share with
other instances of that site when over the limit.  Same for non-tab features
that use WebContents.  I could imagine implementing that more as pools (e.g.,
defined by site), rather than saying "never reuse under any circumstances."

In other words, we might want to leave this out until there's a clear need for
it.

[nasko, 2017/05/03 23:46:17]

nit: DO_NOT

[clamy, 2017/05/04 15:56:19]

I initially thought we might want to change
RenderProcessHostImpl::ShouldTryToUseExistingProcessHost so that when
SiteIsolationPolicy::UseDedicatedProcessesForAllSites() is true (and not in
single process mode), we intialize the SiteInstance with this value instead of
default. But this is probably out-of-scope for this CL, so I'm removing it.

+
+    // In this mode, all instances of the site will be rendered in the same

[Charlie Reis, 2017/05/03 23:28:00]

Is the idea to also use this value for ServiceWorker SiteInstances, to make them
join one (of possibly many) existing processes?  And for process-per-site sites
(e.g., WebUI), we use it for all SiteInstances in the site?

If so, we should rephrase this-- in the ServiceWorker case, we'll set this value
for the ServiceWorker SiteInstance but not "all instances of the site."  The
normal tabs for the site will still get the default value.

Since we're choosing whether to apply this value to one or all SiteInstances for
a site to define different policies, that's a second reason to move this enum to
SiteInstance instead of RenderProcessHost.

[nasko, 2017/05/03 23:46:17]

nit: s/rendererd/hosted/?

[clamy, 2017/05/04 15:56:19]

Done.

For the naming question, I'm planning a different value for ServiceWorkers,
since we also have to take into account pending navigations (I've called it
REUSE_CHECKING_FRAMES_AND_NAVIGATIONS).

If we want, we can eventually name this one REUSE_CHECKING_FRAMES. Note that in
this CL I'm trying not to make changes to the functionality, and in particular
I'm not changing the implementation of process-per-site. I'd rather leave it
named as it is, and change the name once we change the underlying implemetation.
Wdyt?

[Charlie Reis, 2017/05/04 23:12:23]

Works for me, thanks.

+    // RenderProcessHost.
+    PROCESS_PER_SITE,
+
+    // In this mode, subframes will be rendered in a designated
+    // RenderProcessHost.
+    USE_DEFAULT_SUBFRAME_INSTANCE,

[Charlie Reis, 2017/05/03 23:28:00]

s/INSTANCE/PROCESS/, perhaps?

There are multiple default subframe SiteInstances (one per BrowsingInstance),
but only one RenderProcessHost.

[clamy, 2017/05/04 15:56:19]

Done.

+
+    // By default, a new RenderProcessHost will be created unless the process
+    // limit has been reached.

[Charlie Reis, 2017/05/03 23:28:00]

Let's mention that reuse after the limit is random, not based on site.

[clamy, 2017/05/04 15:56:19]

Done.

+    DEFAULT,
+  };
+
+  // Returns a suitable RenderProcessHost to use for |site_instance|. Depending
+  // on the SiteInstance ProcessReusePolicy and its url, this may be an existing

[Charlie Reis, 2017/05/03 23:28:00]

nit: SiteInstance's

[clamy, 2017/05/04 15:56:19]

Done.

+  // RenderProcessHost or a new one.
+  static RenderProcessHost* GetProcessHostForSiteInstance(
+      BrowserContext* browser_context,
+      SiteInstanceImpl* site_instance);
+
   static base::MessageLoop* GetInProcessRendererThreadForTesting();
 
   // This forces a renderer that is running "in process" to shut down.
@@ -295,6 +322,13 @@ class CONTENT_EXPORT RenderProcessHostImpl
   void OnAudioStreamRemoved() override;
   int get_audio_stream_count_for_testing() const { return audio_stream_count_; }
 
+  // Sets the global factory used to create new RenderProcessHosts.  It may be
+  // nullptr, in which case the default RenderProcessHost will be created (this
+  // is the behavior if you don't call this function).  The factory must be set
+  // back to nullptr before it's destroyed; ownership is not transferred.
+  static void set_render_process_host_factory(

[Charlie Reis, 2017/05/03 23:28:00]

Thanks-- this makes more sense here.

+      const RenderProcessHostFactory* rph_factory);
+
  protected:
   // A proxy for our IPC::Channel that lives on the IO thread.
   std::unique_ptr<IPC::ChannelProxy> channel_;
@@ -388,6 +422,12 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // GpuSwitchingObserver implementation.
   void OnGpuSwitched() override;
 
+  // Returns the default subframe RenderProcessHost to use for |site_instance|.
+  static RenderProcessHost* GetDefaultSubframeProcessHost(
+      BrowserContext* browser_context,
+      SiteInstanceImpl* site_instance,
+      bool is_for_guests_only);
+
 #if BUILDFLAG(ENABLE_WEBRTC)
   void OnRegisterAecDumpConsumer(int id);
   void OnUnregisterAecDumpConsumer(int id);