Fix process reuse for dedicated processes when over process limit.

content/browser/site_instance_impl.cc

Index: content/browser/site_instance_impl.cc
diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
index 0e47c20885ad4e5e8b7396062cf192290c32b3fb..82ccd1b24e591977ad53a57db8f2c31040a920be 100644
--- a/content/browser/site_instance_impl.cc
+++ b/content/browser/site_instance_impl.cc
@@ -124,7 +124,7 @@ RenderProcessHost* SiteInstanceImpl::GetProcess() {
     GetContentClient()->browser()->SiteInstanceGotProcess(this);
 
     if (has_site_)
-      LockToOrigin();
+      LockToOriginIfNeeded();
   }
   DCHECK(process_);
 
@@ -161,7 +161,7 @@ void SiteInstanceImpl::SetSite(const GURL& url) {
   }
 
   if (process_) {
-    LockToOrigin();
+    LockToOriginIfNeeded();
 
     // Ensure the process is registered for this site if necessary.
     if (should_use_process_per_site) {
@@ -211,6 +211,10 @@ bool SiteInstanceImpl::HasWrongProcessForURL(const GURL& url) {
   if (IsRendererDebugURL(url))
     return false;
 
+  // Any process can host an about:blank URL.
+  if (url == url::kAboutBlankURL)
+    return false;
+
   // If the site URL is an extension (e.g., for hosted apps or WebUI) but the
   // process is not (or vice versa), make sure we notice and fix it.
   GURL site_url = GetSiteForURL(browsing_instance_->browser_context(), url);
@@ -402,6 +406,38 @@ bool SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
   return false;
 }
 
+// static
+bool SiteInstanceImpl::ShouldLockToOrigin(BrowserContext* browser_context,
+                                          GURL site_url) {
+  if (!DoesSiteRequireDedicatedProcess(browser_context, site_url))
+    return false;
+
+  // Guest processes cannot be locked to its site because guests always have

[Charlie Reis, 2017/06/17 23:13:53]

nit: s/its/their/

[alexmos, 2017/06/19 20:03:59]

Done.

+  // a fixed SiteInstance. The site of GURLs a guest loads doesn't match that
+  // SiteInstance. So we skip locking the guest process to the site.
+  // TODO(ncarter): Remove this exclusion once we can make origin lock per
+  // RenderFrame routing id.
+  if (site_url.SchemeIs(content::kGuestScheme))
+    return false;
+
+  // TODO(creis, nick) https://crbug.com/510588 Chrome UI pages use the same

[Charlie Reis, 2017/06/17 23:13:53]

Oh, I see.  Actually, dbeam@ was just working on a CL
(https://codereview.chromium.org/2931243002/) that may unblock a lot of that, if
I understood correctly.

[alexmos, 2017/06/19 20:03:58]

Ack - thanks for the pointer!

+  // site (chrome://chrome), so they can't be locked because the site being
+  // loaded doesn't match the SiteInstance.
+  if (site_url.SchemeIs(content::kChromeUIScheme))
+    return false;
+
+  // TODO(creis, nick): Until we can handle sites with effective URLs at the
+  // call sites of ChildProcessSecurityPolicy::CanAccessDataForOrigin, we
+  // must give the embedder a chance to exempt some sites to avoid process
+  // kills.
+  if (!GetContentClient()->browser()->ShouldLockToOrigin(browser_context,
+                                                         site_url)) {
+    return false;
+  }
+
+  return true;
+}
+
 void SiteInstanceImpl::RenderProcessHostDestroyed(RenderProcessHost* host) {
   DCHECK_EQ(process_, host);
   process_->RemoveObserver(this);
@@ -422,34 +458,26 @@ void SiteInstanceImpl::RenderProcessExited(RenderProcessHost* host,
     observer.RenderProcessGone(this);
 }
 
-void SiteInstanceImpl::LockToOrigin() {
+void SiteInstanceImpl::LockToOriginIfNeeded() {
+  DCHECK(HasSite());
+
+  // From now on, this process should be considered "tainted" for future
+  // process reuse decisions:
+  // (1) If |site_| required a dedicated process, this SiteInstance's process
+  //     can only host URLs for the same site.
+  // (2) Even if |site_| does not require a dedicated process, this
+  //     SiteInstance's process still cannot be reused to host other sites
+  //     requiring dedicated sites in the future.
+  // We can get here either when we commit a URL into a SiteInstance that does
+  // not yet have a site, or when we create a process for a SiteInstance with a
+  // preassigned site.
+  process_->SetIsUsed();
+
   // TODO(nick): When all sites are isolated, this operation provides strong
   // protection. If only some sites are isolated, we need additional logic to
   // prevent the non-isolated sites from requesting resources for isolated
   // sites. https://crbug.com/509125
-  if (RequiresDedicatedProcess()) {
-    // Guest processes cannot be locked to its site because guests always have
-    // a fixed SiteInstance. The site of GURLs a guest loads doesn't match that
-    // SiteInstance. So we skip locking the guest process to the site.
-    // TODO(ncarter): Remove this exclusion once we can make origin lock per
-    // RenderFrame routing id.
-    if (site_.SchemeIs(content::kGuestScheme))
-      return;
-
-    // TODO(creis, nick) https://crbug.com/510588 Chrome UI pages use the same
-    // site (chrome://chrome), so they can't be locked because the site being
-    // loaded doesn't match the SiteInstance.
-    if (site_.SchemeIs(content::kChromeUIScheme))
-      return;
-
-    // TODO(creis, nick): Until we can handle sites with effective URLs at the
-    // call sites of ChildProcessSecurityPolicy::CanAccessDataForOrigin, we
-    // must give the embedder a chance to exempt some sites to avoid process
-    // kills.
-    if (!GetContentClient()->browser()->ShouldLockToOrigin(
-            browsing_instance_->browser_context(), site_))
-      return;
-
+  if (ShouldLockToOrigin(GetBrowserContext(), site_)) {
     ChildProcessSecurityPolicyImpl* policy =
         ChildProcessSecurityPolicyImpl::GetInstance();
     policy->LockToOrigin(process_->GetID(), site_);