Check for already existing entry when adding to SiteProcessMap.

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 07f3cc9d1eb6137155132c4f146032ea6b1efe7d..a10793e4a2ca4295fb26f2fd507bf061d2902a1a 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -625,12 +625,12 @@ void NavigatorImpl::DidNavigate(
   }
 
   // Update the site of the SiteInstance if it doesn't have one yet, unless
-  // assigning a site is not necessary for this URL.  In that case, the
-  // SiteInstance can still be considered unused until a navigation to a real
-  // page.
+  // assigning a site is not necessary for this URL or the commit was for an
+  // error page.  In that case, the SiteInstance can still be considered unused

[Charlie Reis, 2017/03/13 22:31:33]

Leaving it unassigned for the error page is something we may want to revisit
later (or possibly now?).  It's not terrible, since the error page itself is
under Chrome's control and shouldn't pose a risk to whatever ends up in the
process next, but I do wonder what would happen if you went to an allowed
extension URL next.  Would we end up giving the process bindings?  Would that
result in a kill or a crash because of the mix of non-bindings and bindings in
the same process?

(For example, start with about:blank, then go to the blocked extension URL,
getting here.  We leave the site unassigned.  Then go to a real extension URL--
do we stay in the same process?  If so, do we crash when going back because the
bindings on the NavigationEntry don't match the RFH?)

If that is a problem, we could consider setting the site URL to data: for the
error page, so that it isn't reused later for the next navigation.

[nasko, 2017/03/14 16:56:02]

We correctly transfer it to the extension process :)
The process is no longer used, so it goes away.
 
I tried renderer-initiated direct navigation to the extension allowed URL,
renderer-initiated navigation to server that redirects to the allowed URL and
browser-initiated navigation to the same URL. They all resulted in committing in
the extension process with this CL in place.
 

[Charlie Reis, 2017/03/14 23:46:47]

Ok-- I'm glad you tested the redirect case, since I know that was needed in your
browsertest.  Maybe it's not an issue.  I'm still a bit nervous about it, but
hopefully our plans to change how error pages work can resolve this aspect down
the road.

One other case to test, and then I'll be happy.  :)  What happens if you visit
blocked.html and get to the error page, do a renderer-initiated navigation to a
web page (redirecting if needed, so that we stay in process), then change the
onBeforeRequest listener in the extension to stop blocking, then go back?  Would
we end up loading blocked.html in the process that has now been used for web
pages?

[nasko, 2017/03/15 00:32:06]

Commits as expected in the extension process :). However, I should note that
changing the JS required the extension to be restarted, so it resulted in a new
process. I tried making the JS dynamic, such that it will only block the first
request and allow afterwards. That also resulted in the page to be put in the
extension process.

+  // until a navigation to a real page.
   SiteInstanceImpl* site_instance = render_frame_host->GetSiteInstance();
-  if (!site_instance->HasSite() &&
-      ShouldAssignSiteForURL(params.url)) {
+  if (!site_instance->HasSite() && ShouldAssignSiteForURL(params.url) &&
+      !params.url_is_unreachable) {
     site_instance->SetSite(params.url);
   }