Fix process reuse for dedicated processes when over process limit.

content/browser/site_instance_impl.cc

 // Copyright (c) 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.
 
 #include "content/browser/site_instance_impl.h"
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "content/browser/browsing_instance.h"
 #include "content/browser/child_process_security_policy_impl.h"
@@ skipping 106 matching lines @@
         has_site_) {
       RenderProcessHostImpl::RegisterProcessHostForSite(browser_context,
                                                         process_, site_);
     }
 
     TRACE_EVENT2("navigation", "SiteInstanceImpl::GetProcess",
                  "site id", id_, "process id", process_->GetID());
     GetContentClient()->browser()->SiteInstanceGotProcess(this);
 
     if (has_site_)
-      LockToOrigin();
+      LockToOriginIfNeeded();
   }
   DCHECK(process_);
 
   return process_;
 }
 
 void SiteInstanceImpl::SetSite(const GURL& url) {
   TRACE_EVENT2("navigation", "SiteInstanceImpl::SetSite",
                "site id", id_, "url", url.possibly_invalid_spec());
   // A SiteInstance's site should not change.
@@ skipping 16 matching lines @@
   browsing_instance_->RegisterSiteInstance(this);
 
   // Update the process reuse policy based on the site.
   bool should_use_process_per_site =
       RenderProcessHost::ShouldUseProcessPerSite(browser_context, site_);
   if (should_use_process_per_site) {
     process_reuse_policy_ = ProcessReusePolicy::PROCESS_PER_SITE;
   }
 
   if (process_) {
-    LockToOrigin();
+    LockToOriginIfNeeded();
 
     // Ensure the process is registered for this site if necessary.
     if (should_use_process_per_site) {
       RenderProcessHostImpl::RegisterProcessHostForSite(
           browser_context, process_, site_);
     }
   }
 }
 
 const GURL& SiteInstanceImpl::GetSiteURL() const {
@@ skipping 29 matching lines @@
   // We want to use such a process in the IsSuitableHost check, so we
   // may end up assigning process_ in the GetProcess() call below.
   if (!HasProcess())
     return false;
 
   // If the URL to navigate to can be associated with any site instance,
   // we want to keep it in the same process.
   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);
   return !RenderProcessHostImpl::IsSuitableHost(
       GetProcess(), browsing_instance_->browser_context(), site_url);
 }
 
 scoped_refptr<SiteInstanceImpl>
 SiteInstanceImpl::GetDefaultSubframeSiteInstance() {
   return browsing_instance_->GetDefaultSubframeSiteInstance();
@@ skipping 171 matching lines @@
   // (blob and filesystem) work properly.
   if (GetContentClient()->IsSupplementarySiteIsolationModeEnabled() &&
       GetContentClient()->browser()->DoesSiteRequireDedicatedProcess(
           browser_context, site_url)) {
     return true;
   }
 
   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);
   process_ = nullptr;
 }
 
 void SiteInstanceImpl::RenderProcessWillExit(RenderProcessHost* host) {
   // TODO(nick): http://crbug.com/575400 - RenderProcessWillExit might not serve
   // any purpose here.
   for (auto& observer : observers_)
     observer.RenderProcessGone(this);
 }
 
 void SiteInstanceImpl::RenderProcessExited(RenderProcessHost* host,
                                            base::TerminationStatus status,
                                            int exit_code) {
   for (auto& observer : observers_)
     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_);
   }
 }
 
 }  // namespace content