Enable swapping a frame back in to its parent process

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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/frame_host/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 963 matching lines @@
   // const GURL& current_url = current_instance->site();
   // For now, though, we're in a hybrid model where you only switch
   // SiteInstances if you type in a cross-site URL.  This means we have to
   // compare the entry's URL to the last committed entry's URL.
   NavigationEntry* current_entry = controller.GetLastCommittedEntry();
   if (interstitial_page_) {
     // The interstitial is currently the last committed entry, but we want to
     // 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
   // chrome://crash, however.
   // TODO(creis): Refactor this method so this duplicated code isn't needed.
   // See http://crbug.com/123007.
   if (current_entry &&
       current_entry->IsViewSourceMode() != dest_is_view_source_mode &&
       !IsRendererDebugURL(dest_url)) {
     return SiteInstance::CreateForURL(browser_context, dest_url);
   }
 
   // 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;
   }
 
   // Start the new renderer in a new SiteInstance, but in the current
   // BrowsingInstance.  It is important to immediately give this new
   // SiteInstance to a RenderViewHost (if it is different than our current
   // SiteInstance), so that it is ref counted.  This will happen in
   // CreateRenderView.
@@ skipping 119 matching lines @@
     new_render_frame_host = CreateRenderFrameHost(
         instance, MSG_ROUTING_NONE, MSG_ROUTING_NONE, swapped_out, hidden);
     RenderViewHostImpl* render_view_host =
         new_render_frame_host->render_view_host();
     int proxy_routing_id = MSG_ROUTING_NONE;
 
     // Prevent the process from exiting while we're trying to navigate in it.
     // 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,
                                   opener_route_id,
                                   proxy_routing_id,
                                   for_main_frame_navigation);
     if (success) {
       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());
         success = InitRenderFrame(new_render_frame_host.get());
       }
       if (swapped_out) {
         proxy_hosts_[instance->GetId()]->InitRenderFrameProxy();
       }
     } else if (!swapped_out && pending_render_frame_host_) {
       CancelPending();
@@ skipping 531 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content