Fix process reuse for dedicated processes when over process limit.

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 d18fd32416a5d398395e58b9e776a8c26850380c..c9834e862c1a611c594faf507d6a222d6fe1d223 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -929,6 +929,7 @@ RenderProcessHostImpl::RenderProcessHostImpl(
       sudden_termination_allowed_(true),
       ignore_input_events_(false),
       is_for_guests_only_(is_for_guests_only),
+      can_become_dedicated_process_(true),
       gpu_observer_registered_(false),
       delayed_cleanup_needed_(false),
       within_process_died_observer_(false),
@@ -1760,6 +1761,14 @@ bool RenderProcessHostImpl::MayReuseHost() {
   return GetContentClient()->browser()->MayReuseHost(this);
 }
 
+bool RenderProcessHostImpl::CanBecomeDedicatedProcess() {
+  return can_become_dedicated_process_;
+}
+
+void RenderProcessHostImpl::UnsetCanBecomeDedicatedProcess() {
+  can_become_dedicated_process_ = false;

[alexmos, 2017/06/12 17:35:07]

Note that I'm specifically not calling this in
IncrementServiceWorkerRefCount/IncrementSharedWorkerRefCount.  Shared workers
would need a page to already be committed in the process to be created, meaning
we should've already called this.  Service workers also should be fine:
ServiceWorkerProcessManager::AllocateWorkerProcess will either put the worker
into an existing process which already has a regular page committed, or it will
create a new SiteInstance for a specific URL (causing site_ to be assigned), and
then will immediately call GetProcess(), which will lead to LockToOrigin and
again disallow reuse.

+}
+
 mojom::RouteProvider* RenderProcessHostImpl::GetRemoteRouteProvider() {
   return remote_route_provider_.get();
 }
@@ -2851,13 +2860,27 @@ bool RenderProcessHostImpl::IsSuitableHost(RenderProcessHost* host,
     return false;
 
   // TODO(nick): Consult the SiteIsolationPolicy here. https://crbug.com/513036

[Charlie Reis, 2017/06/12 23:08:17]

Is this TODO resolved now?

[alexmos, 2017/06/14 22:39:04]

Indeed, removed.

-  if (ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
-          host->GetID()) !=
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  if (policy->HasWebUIBindings(host->GetID()) !=
       WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
           browser_context, site_url)) {
     return false;
   }
 
+  // Sites requiring dedicated processes can only reuse a compatible process.
+  if (policy->HasOriginLock(host->GetID())) {
+    // If the process is already dedicated to a site, only allow the destination
+    // URL to reuse this process if the URL's has the same site.
+    return policy->IsLockedToOrigin(host->GetID(), site_url);

[Charlie Reis, 2017/06/12 23:08:17]

I'm a little concerned about TOCTTOU problems here, since HasOriginLock and
IsLockedToOrigin separately acquire and release the AutoLock.  I'm not sure I
can picture a scenario where it's a problem (since once HasOriginLock returns
true, the value of either method isn't going to change), but it feels fragile.

I suppose we could document the risks on the declarations.  Or we could store
which site the process is locked to on RPHI, so we don't need to acquire the
AutoLock to query it.  Thoughts?

[alexmos, 2017/06/14 22:39:05]

Yes, good questions.  Since we always set origin locks on the UI thread, I don't
think this will be a problem in practice.  We could add documentation/DCHECK to
that effect in ChildProcessSecurityPolicyImpl::LockToOrigin.

I did think about this, and in an earlier PS, I actually had a version of this
which simply queried policy->GetOriginLock(id), which we can then check for
is_empty() and for site_url equality.  I ultimately moved away from that, as I
thought it was better to keep origin lock checks encapsulated inside CPSP.

Another option could be for IsLockedToOrigin(id, site_url) to return an enum 
corresponding to the three {no_lock, wrong_lock, right_lock} cases.  And perhaps
rename it to CheckOriginLock.  This makes it a bit more difficult to use, but it
avoids the TOCTTOU concerns completely, and also forces call sites to explicitly
reason about whether or not they care about an empty origin lock.

WDYT?

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

I really like the enum idea, especially because it makes people think about the
various cases when checking the origin lock.

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

Done.

+  } else if (!host->CanBecomeDedicatedProcess() &&
+             SiteInstanceImpl::DoesSiteRequireDedicatedProcess(browser_context,
+                                                               site_url)) {
+    // Otherwise, if this process cannot host a site that requires a dedicated
+    // process (e.g., if it has hosted any other content), it cannot be reused
+    // if the destination site indeed requires a dedicated process.
+    return false;
+  }
+
   return GetContentClient()->browser()->IsSuitableHost(host, site_url);
 }
 
@@ -2902,20 +2925,9 @@ RenderProcessHost* RenderProcessHost::FromID(int render_process_id) {
 bool RenderProcessHost::ShouldTryToUseExistingProcessHost(
     BrowserContext* browser_context,
     const GURL& url) {
-  // This needs to be checked first to ensure that --single-process
-  // and --site-per-process can be used together.
   if (run_renderer_in_process())
     return true;
 
-  // If --site-per-process is enabled, do not try to reuse renderer processes
-  // when over the limit.
-  // TODO(nick): This is overly conservative and isn't launchable. Move this
-  // logic into IsSuitableHost, and check |url| against the URL the process is
-  // dedicated to. This will allow pages from the same site to share, and will
-  // also allow non-isolated sites to share processes. https://crbug.com/513036
-  if (SiteIsolationPolicy::UseDedicatedProcessesForAllSites())
-    return false;
-
   // NOTE: Sometimes it's necessary to create more render processes than
   //       GetMaxRendererProcessCount(), for instance when we want to create
   //       a renderer process for a browser context that has no existing
@@ -2991,11 +3003,10 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSite(
 
   // See if we have an existing process with appropriate bindings for this site.
   // If not, the caller should create a new process and register it.
-  std::string site =
-      SiteInstance::GetSiteForURL(browser_context, url).possibly_invalid_spec();
-  RenderProcessHost* host = map->FindProcess(site);
+  GURL site_url = SiteInstance::GetSiteForURL(browser_context, url);
+  RenderProcessHost* host = map->FindProcess(site_url.possibly_invalid_spec());
   if (host && (!host->MayReuseHost() ||
-               !IsSuitableHost(host, browser_context, url))) {
+               !IsSuitableHost(host, browser_context, site_url))) {

[alexmos, 2017/06/12 17:35:07]

After my IsSuitableHost modifications, a few extension tests (such as
WebNavigationApiTest.UserAction) caught this bug, which caused unneeded process
swaps (when site_url was used for origin lock comparison).

[Charlie Reis, 2017/06/12 23:08:17]

Nice.  Can you give an example of what the previous values were?  I'm not sure I
follow what the bug was.

(It may also be worth mentioning something to this effect in the comment above,
not necessarily with the example.)

[alexmos, 2017/06/14 22:39:05]

Sorry, typo here - this should've just been "when |url| was used for origin lock
comparison."
So, for example, the old code would pass in
  chrome-extension://plbgfcnklnmpjkhjcfeaibohlaohmjhc/a.html?32773
to IsSuitableHost, which would compare that against the origin lock of
  chrome-extension://plbgfcnklnmpjkhjcfeaibohlaohmjhc/
and fail, which would result in returning null from here and not staying in the
existing extension process.

I added a short comment about this; I think it's just a straightforward case of
passing the wrong thing.  It'd be nice if site URLs used a different type to
avoid these kinds of bugs, since this isn't the first time it happened (I think
Nick fixed another one somewhere).

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

I see now-- thanks!
Agreed.  We wanted to introduce a notion of a principal earlier (I think in
https://crbug.com/109792 and perhaps another bug having to do with
BrowserPlugin's guest SiteInstances), but it was vetoed at the time.  I think
we'll want to revisit that, especially as (1) the definition of site changes to
sometimes include origins and/or ports, (2) we want to introduce knowledge of
StoragePartition to support cross-process navigations and OOPIFs in guests, and
(3) we continue to discover more reasons that GURL or Origin alone are not
sufficient to represent a security principal.

I think this will be a fun project, once we have a chance.  Want to file a bug
for it?

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

Done - filed https://bugs.chromium.org/p/chromium/issues/detail?id=734722

     // The registered process does not have an appropriate set of bindings for
     // the url.  Remove it from the map so we can register a better one.
     RecordAction(