Enable swapping a frame back in to its parent process

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 8d557ace789b3b3c784bab09ebaa946467840a5b..d11de6a06746dc27aee1d509480e60fcbf191447 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -981,18 +981,6 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
     // compare against the last non-interstitial entry.
     current_entry = controller.GetEntryAtOffset(-1);
   }
-  // If there is no last non-interstitial entry (and current_instance already
-  // has a site), then we must have been opened from another tab.  We want
-  // to compare against the URL of the page that opened us, but we can't
-  // get to it directly.  The best we can do is check against the site of
-  // the SiteInstance.  This will be correct when we intercept links and
-  // script-based navigations, but for now, it could place some pages in a
-  // new process unnecessarily.  We should only hit this case if a page tries
-  // to open a new tab to an interstitial-inducing URL, and then navigates
-  // the page to a different same-site URL.  (This seems very unlikely in
-  // practice.)
-  const GURL& current_url = (current_entry) ? current_entry->GetURL() :

[Nate Chapin, 2014/09/30 21:44:47]

This code and comment are really old (pre-opensourcing). It's also wrong for
site isolation, as current_entry->GetURL() will return the main frame's url,
which may not be from the same site as the frame actually being navigated,
leading to wrong SiteInstance selection. Moved down a few lines and simplified.

[Charlie Reis, 2014/09/30 23:02:43]

Ah, this is tricky.  We should fix this in its own CL.

The comment is definitely stale, and I agree that the code does the wrong thing
by looking at the main frame's URL.  Good catch.

It's tricky to get this right, though, since we're not strict about which URLs
are loaded in most SiteInstances.  If you have a SiteInstance for a.com and
click a link to b.com, it stays in the same SiteInstance.  If the user then
types another b.com URL into the omnibox (etc), we don't want to force a swap.

Instead, perhaps we should grab FrameTreeNode's current_frame_url() instead of
the entry's URL?  I think that should be zero behavior change for normal Chrome,
and fix the subframe case.

[Nate Chapin, 2014/09/30 23:19:44]

I can try that. I had also considered making the behavior dependent on
kSitePerProcess being specified, but that's messier.

-      current_instance->GetSiteURL();
 
   // View-source URLs must use a new SiteInstance and BrowsingInstance.
   // We don't need a swap when going from view-source to a debug URL like
@@ -1008,6 +996,7 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
   // Use the current SiteInstance for same site navigations, as long as the
   // process type is correct.  (The URL may have been installed as an app since
   // the last time we visited it.)
+  const GURL& current_url = current_instance->GetSiteURL();
   if (SiteInstance::IsSameWebSite(browser_context, current_url, dest_url) &&
       !current_site_instance->HasWrongProcessForURL(dest_url)) {
     return current_instance;
@@ -1147,13 +1136,13 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
     // Otherwise, if the new RFH is swapped out already, store it.
     if (!swapped_out) {
       new_render_frame_host->GetProcess()->AddPendingView();
-    } else {
-      proxy = new RenderFrameProxyHost(
+    } else if (!proxy) {
+      RenderFrameProxyHost* new_proxy = new RenderFrameProxyHost(
           new_render_frame_host->GetSiteInstance(), frame_tree_node_);
-      proxy_hosts_[instance->GetId()] = proxy;
-      proxy_routing_id = proxy->GetRoutingID();
+      proxy_hosts_[instance->GetId()] = new_proxy;
+      proxy_routing_id = new_proxy->GetRoutingID();
       if (frame_tree_node_->IsMainFrame())
-        proxy->TakeFrameHostOwnership(new_render_frame_host.Pass());
+        new_proxy->TakeFrameHostOwnership(new_render_frame_host.Pass());
     }
 
     bool success = InitRenderView(render_view_host,
@@ -1164,6 +1153,10 @@ int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
       if (frame_tree_node_->IsMainFrame()) {
         // Don't show the main frame's view until we get a DidNavigate from it.
         render_view_host->GetView()->Hide();
+      } else if (proxy) {
+        proxy->SwapOut(new_render_frame_host.get());

[Nate Chapin, 2014/09/30 21:44:47]

This case is where we have a RenderFrameProxyHost, it wasn't created above, and
there is no RenderFrameImpl associated with the RenderFrameProxyHost. i.e., this
is the case where we are doing a real navigate in what is currently a proxy.
Tell it to swap in a real frame.

[Charlie Reis, 2014/09/30 23:02:43]

I don't think we want to do the swap until the new frame commits.

[Nate Chapin, 2014/09/30 23:19:44]

Hrm. I guess that's more accurate.

[nasko, 2014/10/08 16:42:58]

When we are creating a frame and there exists a proxy for the same SiteInstance,
we need to send the routing id of the proxy as part of InitRenderFrame. This
will allow the RenderFrameImpl to know which RenderFrameProxy to swap with at
time of didCommitProvisionalLoad.

+        delete proxy;
+        proxy_hosts_.erase(instance->GetId());
       } else if (!swapped_out) {
         // Init the RFH, so a RenderFrame is created in the renderer.
         DCHECK(new_render_frame_host.get());