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..8ed1a7d28e8d64d523d28cfd880a6fdb2e4f2824 100644
--- a/content/browser/site_instance_impl.cc
+++ b/content/browser/site_instance_impl.cc
@@ -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)

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

Several tests (like DumpAccessibilityTreeTest.AccessibilityA or other
accessibility tests) expect to navigate A->about:blank->A and for all this to
end up in same process (i.e., no swap for about:blank).  Without this check,
with --site-per-process, we'd go into IsSuitableHost, which would compare the
origin lock against "about:" (site_url for about:blank) and fail.  So I added a
special case here.

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

This makes me a bit nervous.  In other places, we've had to deal with
about:srcdoc and data: URLs alongside about:blank (e.g., later in
DetermineSiteInstanceForURL).  Those seem like they would have the same problem.

Is there a way we can reorder or share code to avoid having to copy/paste all
these cases in both places?

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

This is a bit tricky.  Just to clarify, my intent here was to fix only
browser-initiated navigations (the tests were doing this via NavigateToURL), and
the DetermineSiteInstanceForURL special cases only matter for renderer-initiated
navigations (since they kick in only when source_instance is not null).  The
renderer-initiated navigations don't really reach the change here: subframes
will replace about:blank, data:, and other unique-origin URLs with the source
SiteInstance's site URL in CanSubframeSwapProcess before reaching
IsRendererTransferNeededForNavigation -> IsCurrentlySameSite ->
HasWrongProcessForURL.  For top-level frames, we will hit the logic you
mentioned in DetermineSiteInstanceForURL before ever getting here.  But for
browser-initiated navigations, DetermineSiteInstanceForURL doesn't use the
about:blank logic (since source_instance is null), we fall through to check
IsCurrentlySameSite, which first checks HasWrongProcessForURL and then
SiteInstance::IsSameWebSite.  Before this CL, HasWrongProcessForURL was false
(i.e., process was good), and SiteInstance::IsSameWebSite handled this case
here:

  // If the destination url is just a blank page, we treat them as part of the
  // same site.
  GURL blank_page(url::kAboutBlankURL);
  if (dest_url == blank_page)
    return true;

After this CL, the origin locks caused HasWrongProcessForURL to return true
before we could get to that about:blank special case in IsSameWebSite and say
that it's really same-site.  So the reason I only covered about:blank here was
to mirror the case in IsSameWebSite.

I could merge the about:blank usage in HasWrongProcessForURL and in
IsSameWebSite into something like ShouldConsiderURLSameSite, but do you think
this is worth it for just about:blank?  I don't think we want to do this for
data:, which isn't same-site, and it feels safer if HasWrongProcessForURL forces
a process swap for a browser-initiated data: navigation from a site with a
dedicated process.  (Also, actually handling it and returning false would have
no effect, as  it would force a swap anyway later on, because IsSameWebSite will
return false.)  It also doesn't seem like you can get a browser-initiated
about:srcdoc navigation, can you?

It's possible that we could avoid this special case and just force a process
swap for browser-initiated about:blank navigations (e.g., if user types in
about:blank into omnibox), and fix the accessibility test harness to expect the
process swap, though I don't know how difficult that'd be, and swapping on
about:blank still seems a bit wasteful.

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

Interesting points.  I think you're right that we can keep this as you have it. 
I'm sure I won't remember the distinction between this and the
renderer-initiated case (which does care about data: and about:srcdoc), so maybe
there's a way to leave a short explanation in a comment why checking about:blank
is sufficient here?

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

Comment added - please let me know if it's helpful enough.

[Charlie Reis, 2017/06/28 00:08:37]

Thanks-- that works!

+    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);
@@ -454,6 +458,18 @@ void SiteInstanceImpl::LockToOrigin() {
         ChildProcessSecurityPolicyImpl::GetInstance();
     policy->LockToOrigin(process_->GetID(), site_);
   }
+
+  // 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
+  // predetermined site.
+  process_->UnsetCanBecomeDedicatedProcess();

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

I found it confusing that this was inside "LockToOrigin" but relies on that
method being called in all cases, including normal web sites.  Maybe we should
call it MaybeLockToOrigin or LockToOriginIfNeeded?

There almost might be a bug here-- there are a bunch of early returns in the
block above.  Should this line be the first line of the method, so that it
happens regardless?

(I wondered if it should be done before the call to this method, since it's more
related to the commit/set-site behavior than the origin lock behavior, but maybe
that would require too much duplicating logic across two call sites.)

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

Great questions, and I agree with your concerns.  I moved up setting of the flag
to be done as first thing in the method and renamed it to LockToOriginIfNeeded. 
And yes, I put it here because I didn't want to duplicate the logic/comment
across the two call sites, and was also concerned with us missing a new call
site down the road.

[alexmos, 2017/06/15 16:40:54]

Hmm, the trybots show that this wasn't as straightforward as I hoped.  Moving it
up exposed an issue with these early return cases, such as for WebUI.  These
return true from RequiresDedicatedProcess (when running with --site-per-process
for WebUI), yet they don't set the origin lock.  My new check in IsSuitableHost
would incorrectly return false in those cases, since from its point of view, the
process looks like it's "used" and the site_url requires a dedicated process. 
I'm working on a fix.

[alexmos, 2017/06/15 17:27:58]

I've just uploaded the fix (PS8), which is essentially to share the logic of
whether an origin lock is required between LockToOrigin and IsSuitableHost. 
Let's see what the trybots say.

 }
 
 }  // namespace content