Move process-per-site logic from BrowsingInstance to RenderProcessHost.

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 8e759337fd632a9af30667f8d2ec36c1a8890b58..f309f9adfe228d2f5ebef616257128e097996ab9 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -91,6 +91,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_process_host_factory.h"
 #include "content/public/browser/resource_context.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_ui_controller_factory.h"
@@ -208,6 +209,11 @@ class RendererURLRequestContextSelector
 base::LazyInstance<IDMap<RenderProcessHost> >::Leaky
     g_all_hosts = LAZY_INSTANCE_INITIALIZER;
 
+// The global map of BrowsingContexts to SiteProcessMaps, allowing us to
+// implement process-per-site mode within each BrowsingContext.
+base::LazyInstance<RenderProcessHostImpl::ContextSiteProcessMap>::Leaky
+    g_context_site_process_map_ = LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
 // Stores the maximum number of renderer processes the content module can
@@ -338,7 +344,7 @@ RenderProcessHostImpl::~RenderProcessHostImpl() {
   }
 
   ClearTransportDIBCache();
-  UnregisterHost(GetID());
+  UnregisterHost(GetID(), this);
 }
 
 void RenderProcessHostImpl::EnableSendQueue() {
@@ -1071,7 +1077,7 @@ void RenderProcessHostImpl::Cleanup() {
 
     // Remove ourself from the list of renderer processes so that we can't be
     // reused in between now and when the Delete task runs.
-    g_all_hosts.Get().Remove(GetID());
+    UnregisterHost(GetID(), this);
   }
 }
 
@@ -1130,9 +1136,34 @@ void RenderProcessHostImpl::RegisterHost(int host_id, RenderProcessHost* host) {
 }
 
 // static
-void RenderProcessHostImpl::UnregisterHost(int host_id) {
+void RenderProcessHostImpl::UnregisterHost(int host_id,
+                                           RenderProcessHost* host) {
   if (g_all_hosts.Get().Lookup(host_id))
     g_all_hosts.Get().Remove(host_id);
+
+  // Look up the map of site to process for the given browser_context,
+  // in case we need to remove this process from it.  It will be registered
+  // under any sites it rendered that use process-per-site mode.
+  RenderProcessHostImpl::SiteProcessMap* map =
+      &g_context_site_process_map_.Get()[host->GetBrowserContext()];

[awong, 2012/06/27 00:26:54]

Having a global map of BrowserContext objects -> data seems odd.  Would it be
better to user browser_context->SetUserData()? I'm not sure what the rules are
around adding more keys into the context though.

[Charlie Reis, 2012/06/27 20:53:43]

That would be possible, but it also requires adding a new class that inherits
from SupportsUserData::Data.  At the moment, I'm just moving the global state
from BrowsingInstance to RenderProcessHostImpl.

I'm inclined to leave it as is for the moment, but I can switch it if others
prefer.

[awong, 2012/06/27 21:24:40]

sgtm

+  // Find all instances of this process in the map, to remove.
+  std::set<std::string> sites;
+  for (RenderProcessHostImpl::SiteProcessMap::const_iterator i = map->begin();
+      i != map->end();
+      i++) {
+    if (i->second == host)
+      sites.insert(i->first);
+  }
+  // Remove them from the map.

[awong, 2012/06/27 00:26:54]

I'd just merge this with the previous comment. This is getting a bit too close
to redescribing what the code says.

[Charlie Reis, 2012/06/27 20:53:43]

Done.

+  for (std::set<std::string>::iterator i = sites.begin();
+      i != sites.end();
+      i++) {
+    RenderProcessHostImpl::SiteProcessMap::iterator iter = map->find(*i);
+    if (iter != map->end()) {
+      DCHECK_EQ(iter->second, host);
+      map->erase(iter);
+    }
+  }
 }
 
 // static
@@ -1235,10 +1266,76 @@ RenderProcessHost* RenderProcessHost::GetExistingProcessHost(
   return NULL;
 }
 
+// static
+bool RenderProcessHost::ShouldUseProcessPerSite(BrowserContext* browser_context,
+                                                const GURL& url) {
+  // Returns true if we should use the process-per-site model.  This will be
+  // the case if the --process-per-site switch is specified, or in
+  // process-per-site-instance for particular sites (e.g., WebUI).
+
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+  if (command_line.HasSwitch(switches::kProcessPerSite))
+    return true;
+
+  // We want to consolidate particular sites like WebUI when we are using
+  // process-per-tab or process-per-site-instance models.
+  // Note that --single-process is handled in ShouldTryToUseExistingProcessHost.
+
+  if (content::GetContentClient()->browser()->
+          ShouldUseProcessPerSite(browser_context, url)) {
+    return true;
+  }
+
+  // DevTools pages have WebUI type but should not reuse the same host.
+  WebUIControllerFactory* factory =
+      content::GetContentClient()->browser()->GetWebUIControllerFactory();
+  if (factory &&
+      factory->UseWebUIForURL(browser_context, url) &&
+      !url.SchemeIs(chrome::kChromeDevToolsScheme)) {
+    return true;
+  }
+
+  // In all other cases, don't use process-per-site logic.
+  return false;
+}
+
+// static
+RenderProcessHost* RenderProcessHost::GetProcessHostForSite(
+    BrowserContext* browser_context,
+    const GURL& url) {
+  // Look up the map of site to process for the given browser_context.
+  RenderProcessHostImpl::SiteProcessMap* map =
+      &g_context_site_process_map_.Get()[browser_context];
+
+  // See if we have an existing process for this site.
+  GURL site_url = SiteInstanceImpl::GetSiteForURL(browser_context, url);

[awong, 2012/06/27 00:26:54]

std::string site = SiteInstanceImpl::GetSiteForURL(browser_context, url)
    .possibly_invalid_spec();

is what is used in browsing_instance.cc.  Make them consistent here and
elsewhere?  (I personally prefer not introducing the extra var, but up to you).

[Charlie Reis, 2012/06/27 20:53:43]

Done.

+  std::string site = site_url.possibly_invalid_spec();
+  RenderProcessHostImpl::SiteProcessMap::iterator i = map->find(site);
+  if (i != map->end())
+    return i->second;
+
+  // Otherwise, the caller should create a new process and register it.
+  return NULL;
+}
+
+void RenderProcessHost::RegisterProcessHostForSite(
+    BrowserContext* browser_context,
+    RenderProcessHost* process,
+    const GURL& url) {
+  // Look up the map of site to process for the given browser_context.
+  RenderProcessHostImpl::SiteProcessMap* map =
+      &g_context_site_process_map_.Get()[browser_context];
+
+  GURL site_url = SiteInstanceImpl::GetSiteForURL(browser_context, url);
+  std::string site = site_url.possibly_invalid_spec();
+  if (!site.empty())
+    (*map)[site] = process;

[awong, 2012/06/27 00:26:54]

What happens if the site is empty?  The process is just never recorded?

[Charlie Reis, 2012/06/27 20:53:43]

Right.  I didn't think we would want to group all degenerate cases into the same
process.

Then again, we didn't have that check before in BrowsingInstance.  Perhaps (in
the interest of having a minimal functional change) I should leave the empty
check out.  Updated.

[awong, 2012/06/27 21:24:40]

Cool.  Also maybe a comment explaining why the behavior makes sense if
site.empty() would be helpful to future readers?

[Charlie Reis, 2012/06/27 21:42:30]

Not sure there's a clear answer, so I added a TODO comment to investigate.

+}
+
 void RenderProcessHostImpl::ProcessDied(base::ProcessHandle handle,
-                                           base::TerminationStatus status,
-                                           int exit_code,
-                                           bool was_alive) {
+                                        base::TerminationStatus status,
+                                        int exit_code,
+                                        bool was_alive) {
   // Our child process has died.  If we didn't expect it, it's a crash.
   // In any case, we need to let everyone know it's gone.
   // The OnChannelError notification can fire multiple times due to nested sync