Implement ProcessReusePolicy for SiteInstances

content/browser/renderer_host/render_process_host_impl.cc

 // Copyright 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.
 
 // Represents the browser side of the browser <--> renderer communication
 // channel. There will be one RenderProcessHost per renderer process.
 
 #include "content/browser/renderer_host/render_process_host_impl.h"
 
 #include <algorithm>
@@ skipping 106 matching lines @@
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/renderer_host/render_widget_helper.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_view_frame_subscriber.h"
 #include "content/browser/renderer_host/text_input_client_message_filter.h"
 #include "content/browser/resolve_proxy_msg_helper.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #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"
 #include "content/browser/tracing/trace_message_filter.h"
 #include "content/browser/websockets/websocket_manager.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/common/child_process_host_impl.h"
 #include "content/common/child_process_messages.h"
 #include "content/common/content_switches_internal.h"
 #include "content/common/frame_messages.h"
@@ skipping 114 matching lines @@
 
 #if defined(OS_WIN)
 #define IntToStringType base::IntToString16
 #else
 #define IntToStringType base::IntToString
 #endif
 
 namespace content {
 namespace {
 
+const RenderProcessHostFactory* g_render_process_host_factory_ = nullptr;
 const char kSiteProcessMapKeyName[] = "content_site_process_map";
 
 #if BUILDFLAG(ENABLE_WEBRTC)
 const base::FilePath::CharType kAecDumpFileNameAddition[] =
     FILE_PATH_LITERAL("aec_dump");
 #endif
 
 void CacheShaderInfo(int32_t id, base::FilePath path) {
   if (GetShaderCacheFactorySingleton())
     GetShaderCacheFactorySingleton()->SetCacheInfo(id, path);
@@ skipping 171 matching lines @@
   void Release(int old_route_id) {
     session_storage_namespaces_awaiting_close_->erase(old_route_id);
   }
 
  private:
   std::unique_ptr<std::map<int, SessionStorageNamespaceMap>>
       session_storage_namespaces_awaiting_close_;
   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) {
   MemoryCoordinatorImpl::GetInstance()->CreateHandle(render_process_id,
                                                      std::move(request));
 }
 
 // Forwards service requests to Service Manager since the renderer cannot launch
 // out-of-process services on is own.
 template <typename R>
@@ skipping 1158 matching lines @@
   ++audio_stream_count_;
   UpdateProcessPriority();
 }
 
 void RenderProcessHostImpl::OnAudioStreamRemoved() {
   DCHECK_GT(audio_stream_count_, 0);
   --audio_stream_count_;
   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_;
 }
 
 StoragePartition* RenderProcessHostImpl::GetStoragePartition() const {
   return storage_partition_impl_;
 }
 
 static void AppendCompositorCommandLineFlags(base::CommandLine* command_line) {
   command_line->AppendSwitchASCII(
@@ skipping 1074 matching lines @@
 
   // Only register valid, non-empty sites.  Empty or invalid sites will not
   // use process-per-site mode.  We cannot check whether the process has
   // appropriate bindings here, because the bindings have not yet been granted.
   std::string site =
       SiteInstance::GetSiteForURL(browser_context, url).possibly_invalid_spec();
   if (!site.empty())
     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.
   if (!base::GlobalHistogramAllocator::Get())
     return;
 
   // Get handle to the renderer process. Stop if there is none.
   base::ProcessHandle destination = GetHandle();
   if (destination == base::kNullProcessHandle)
     return;
@@ skipping 308 matching lines @@
       static_cast<SessionStorageHolder*>(GetUserData(kSessionStorageHolderKey));
   if (!holder)
     return;
   holder->Release(old_route_id);
 }
 
 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(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&RenderProcessHostImpl::RegisterAecDumpConsumerOnUIThread,
                  weak_factory_.GetWeakPtr(), id));
 }
 
 void RenderProcessHostImpl::OnUnregisterAecDumpConsumer(int id) {
   BrowserThread::PostTask(
@@ skipping 70 matching lines @@
   LOG(ERROR) << "Terminating render process for bad Mojo message: " << error;
 
   // The ReceivedBadMessage call below will trigger a DumpWithoutCrashing.
   // Capture the error message in a crash key value.
   base::debug::ScopedCrashKey error_key_value("mojo-message-error", error);
   bad_message::ReceivedBadMessage(render_process_id,
                                   bad_message::RPH_MOJO_PROCESS_ERROR);
 }
 
 }  // namespace content