Implement ProcessReusePolicy for SiteInstances

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 5f240939bd6b287f24c74995c3fe69e609443e94..1e58ac50fdfe029592438aeb42773482283e5e69 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -124,6 +124,7 @@
 #include "content/browser/service_worker/service_worker_dispatcher_host.h"
 #include "content/browser/shared_worker/shared_worker_message_filter.h"
 #include "content/browser/shared_worker/worker_storage_partition.h"
+#include "content/browser/site_instance_impl.h"
 #include "content/browser/speech/speech_recognition_dispatcher_host.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/browser/streams/stream_context.h"
@@ -258,6 +259,7 @@
 namespace content {
 namespace {
 
+const RenderProcessHostFactory* g_render_process_host_factory_ = nullptr;
 const char kSiteProcessMapKeyName[] = "content_site_process_map";
 
 #if BUILDFLAG(ENABLE_WEBRTC)
@@ -449,6 +451,63 @@ class SessionStorageHolder : public base::SupportsUserData::Data {
   DISALLOW_COPY_AND_ASSIGN(SessionStorageHolder);
 };
 
+const void* const kDefaultSubframeProcessHostHolderKey =
+    &kDefaultSubframeProcessHostHolderKey;
+
+class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
+                                         public RenderProcessHostObserver {
+ public:
+  explicit DefaultSubframeProcessHostHolder(BrowserContext* browser_context)
+      : browser_context_(browser_context) {}
+  ~DefaultSubframeProcessHostHolder() override {}
+
+  // Gets the correct render process to use for this SiteInstance.
+  RenderProcessHost* GetProcessHost(SiteInstance* site_instance,
+                                    bool is_for_guests_only) {
+    StoragePartition* default_partition =
+        BrowserContext::GetDefaultStoragePartition(browser_context_);
+    StoragePartition* partition =
+        BrowserContext::GetStoragePartition(browser_context_, site_instance);
+
+    // Is this the default storage partition? If it isn't, then just give it its
+    // own non-shared process.
+    if (partition != default_partition || is_for_guests_only) {
+      RenderProcessHostImpl* host = new RenderProcessHostImpl(
+          browser_context_, static_cast<StoragePartitionImpl*>(partition),
+          is_for_guests_only);
+      host->SetIsNeverSuitableForReuse();

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

What about different GuestView instances that share a StoragePartition? They
need to be in the same process. 

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

Following the removing of is_for_guest only asked below, I now keep a map of
default subframe RPH per StoragePartition. So if different instances share a
StoragePartition they should end up in the same RPH. Is that what you had in
mind?

+      return host;
+    }
+
+    if (host_) {
+      // If we already have a shared host for the default storage partition, use

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

nit: I'd put the comment above the if statement and skip having {}.
nit: s/storage partition/StoragePartition/

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

Done.

+      // it.
+      return host_;
+    }
+
+    host_ = new RenderProcessHostImpl(
+        browser_context_, static_cast<StoragePartitionImpl*>(partition),
+        false /* for guests only */);
+    host_->SetIsNeverSuitableForReuse();
+    host_->AddObserver(this);
+
+    return host_;
+  }
+
+  void RenderProcessHostDestroyed(RenderProcessHost* host) override {

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

nit: Add a comment above that this is an implementation of the
RenderProcessHostObserver interface.

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

Done.

+    DCHECK_EQ(host_, host);
+    host_->RemoveObserver(this);
+    host_ = nullptr;
+  }
+
+ private:
+  BrowserContext* browser_context_;
+
+  // The default subframe render process used for the default storage partition
+  // of this BrowserContext.
+  RenderProcessHostImpl* host_ = nullptr;
+};
+
 void CreateMemoryCoordinatorHandle(
     int render_process_id,
     mojom::MemoryCoordinatorHandleRequest request) {
@@ -1627,6 +1686,11 @@ void RenderProcessHostImpl::OnAudioStreamRemoved() {
   UpdateProcessPriority();
 }
 
+void RenderProcessHostImpl::set_render_process_host_factory(
+    const RenderProcessHostFactory* rph_factory) {
+  g_render_process_host_factory_ = rph_factory;
+}
+
 bool RenderProcessHostImpl::IsForGuestsOnly() const {
   return is_for_guests_only_;
 }
@@ -2721,6 +2785,51 @@ void RenderProcessHostImpl::RegisterProcessHostForSite(
     map->RegisterProcess(site, process);
 }
 
+// static
+RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
+    BrowserContext* browser_context,
+    SiteInstanceImpl* site_instance) {
+  const GURL url = site_instance->GetSiteURL();

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

nit: site_url
(This is a meaningful difference in what the GURL is.)

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

Done.

+  ProcessReusePolicy process_reuse_policy =
+      site_instance->process_reuse_policy();
+  bool is_for_guests_only = url.SchemeIs(kGuestScheme);
+  RenderProcessHost* render_process_host = nullptr;
+
+  // First, attempt to reuse an existing RenderProcessHost if applicable.

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

nit: s/applicable/necessary/
(We may also reuse when over the limit, but that's not handled in this block.)

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

Done.

+  switch (process_reuse_policy) {
+    case ProcessReusePolicy::PROCESS_PER_SITE:
+      render_process_host = GetProcessHostForSite(browser_context, url);
+      break;
+    case ProcessReusePolicy::USE_DEFAULT_SUBFRAME_INSTANCE:
+      render_process_host = GetDefaultSubframeProcessHost(

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

DCHECK(SiteIsolationPolicy::IsTopDocumentIsolationEnabled()), perhaps?

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

Done.

+          browser_context, site_instance, is_for_guests_only);
+      break;
+    default:
+      break;
+  }
+
+  if (!render_process_host &&

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

Please keep the old comment:
// If not (or if none found), see if we should reuse an existing process.

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

Done.

+      process_reuse_policy != ProcessReusePolicy::DONT &&
+      ShouldTryToUseExistingProcessHost(browser_context, url)) {
+    render_process_host = GetExistingProcessHost(browser_context, url);
+  }
+
+  // Otherwise (or if that fails), create a new one.
+  if (!render_process_host) {
+    if (g_render_process_host_factory_) {
+      render_process_host =
+          g_render_process_host_factory_->CreateRenderProcessHost(
+              browser_context, site_instance);
+    } else {
+      StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(
+          BrowserContext::GetStoragePartition(browser_context, site_instance));
+      render_process_host = new RenderProcessHostImpl(
+          browser_context, partition, is_for_guests_only);
+    }
+  }
+  return render_process_host;
+}
+
 void RenderProcessHostImpl::CreateSharedRendererHistogramAllocator() {
   // Create a persistent memory segment for renderer histograms only if
   // they're active in the browser.
@@ -3049,6 +3158,23 @@ void RenderProcessHostImpl::OnGpuSwitched() {
   RecomputeAndUpdateWebKitPreferences();
 }
 
+// static
+RenderProcessHost* RenderProcessHostImpl::GetDefaultSubframeProcessHost(
+    BrowserContext* browser_context,
+    SiteInstanceImpl* site_instance,
+    bool is_for_guests_only) {

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

Thinking more about this, does this bool param make sense in the context of
subframes? Guests don't have subframes that live in separate processes. We want
to change that, but it will be very big undertaking.
I think it will be much easier to reason about this code if we just skip that
parameter.

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

Done.

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

I'm pretty nervous about this change, for two reasons.

1) This CL is mostly about moving code, so it's probably better not to change
behavior as we do it.  (If we decide we want to make the change, I think it's
better to do it in a separate CL.)

2) I'm not actually sure which approach is safer.  This was added in Avi's
https://codereview.chromium.org/2573213002.  From talking with Nick, I don't
think they were considering whether TDI would create a subframe process for
guests-- just what plumbing was needed to create a RPHI.  It's true that we
prevent OOPIFs at a different layer, but we're kind of considering changing that
at some point.  Assuming this is false here means we're making the
OOPIFs-in-guests policy decision in two places, and we might forget this one if
we change things later.

It seems like the risk with Avi's approach is that it might make us create a
subframe process for guests in TDI mode and bypass the other no-OOPIFs-in-guests
policy, but that doesn't seem to happen in practice (I just tested).

I think I'd recommend undoing this part of the change and sticking to only
moving the code for now.  We can discuss the tradeoffs of the change on a
separate CL if we want to do it.

[nasko, 2017/05/05 05:27:23]

As I pointed out in one of my comments, I added the comments in this file before
I realized it is a move. I definitely agree that this CL should just move the
code around to minimize changes. The bool can always be investigated separately.
I just reviewed this file as brand new code initially.

+  DefaultSubframeProcessHostHolder* holder =
+      static_cast<DefaultSubframeProcessHostHolder*>(
+          browser_context->GetUserData(&kDefaultSubframeProcessHostHolderKey));
+  if (!holder) {
+    holder = new DefaultSubframeProcessHostHolder(browser_context);
+    browser_context->SetUserData(kDefaultSubframeProcessHostHolderKey,
+                                 base::WrapUnique(holder));
+  }
+
+  return holder->GetProcessHost(site_instance, is_for_guests_only);
+}
+
 #if BUILDFLAG(ENABLE_WEBRTC)
 void RenderProcessHostImpl::OnRegisterAecDumpConsumer(int id) {
   BrowserThread::PostTask(