Start using RenderFrameProxyHost objects.

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 9973a8657557899523f41b4e45c838445fc7a3ce..930f86b2bbbb402483996c64d13182e80c63b138 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -538,7 +538,10 @@ void RenderFrameHostManager::SwapOutOldPage() {
 
   RenderFrameProxyHost* proxy = new RenderFrameProxyHost(
       render_frame_host_->GetSiteInstance(), frame_tree_node_);
-  proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;
+  std::pair<RenderFrameProxyHostMap::iterator, bool> result =
+      proxy_hosts_.insert(std::make_pair(
+          render_frame_host_->GetSiteInstance()->GetId(), proxy));
+  CHECK(result.second) << "Inserting a duplicate item.";
 
   // Tell the old frame it is being swapped out.  This will fire the unload
   // handler in the background (without firing the beforeunload handler a second
@@ -909,24 +912,13 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
   if (frame_routing_id == MSG_ROUTING_NONE)
     frame_routing_id = site_instance->GetProcess()->GetNextRoutingID();
 
-  // Create a RVH for main frames, or find the existing one for subframes.
+  // Create a RVH for the new frame or find an existing one.
   FrameTree* frame_tree = frame_tree_node_->frame_tree();
-  RenderViewHostImpl* render_view_host = NULL;
-  if (frame_tree_node_->IsMainFrame()) {
-    render_view_host = frame_tree->CreateRenderViewHostForMainFrame(
+  RenderViewHostImpl* render_view_host =
+      frame_tree->GetRenderViewHost(site_instance);
+  if (!render_view_host) {
+    render_view_host = frame_tree->CreateRenderViewHost(
         site_instance, view_routing_id, frame_routing_id, swapped_out, hidden);
-  } else {
-    render_view_host = frame_tree->GetRenderViewHostForSubFrame(site_instance);
-
-    // If we haven't found a RVH for a subframe RFH, it's because we currently
-    // do not create top-level RFHs for pending subframe navigations.  Create
-    // the RVH here for now.
-    // TODO(creis): Mirror the frame tree so this check isn't necessary.
-    if (!render_view_host) {
-      render_view_host = frame_tree->CreateRenderViewHostForMainFrame(
-          site_instance, view_routing_id, frame_routing_id, swapped_out,
-          hidden);
-    }
   }
 
   // TODO(creis): Pass hidden to RFH.
@@ -940,14 +932,20 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
   return render_frame_host.Pass();
 }
 
-int RenderFrameHostManager::CreateRenderFrame(
-    SiteInstance* instance,
-    int opener_route_id,
-    bool swapped_out,
-    bool hidden) {
+int RenderFrameHostManager::CreateRenderFrame(SiteInstance* instance,
+                                              int opener_route_id,
+                                              bool swapped_out,
+                                              bool for_main_frame,
+                                              bool hidden) {
   CHECK(instance);
   DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden.
 
+  // TODO(nasko): Remove the following CHECK once cross-site navigation no
+  // longer relies on swapped out RFH for the top-level frame.
+  if (!frame_tree_node_->IsMainFrame()) {
+    CHECK(!swapped_out);
+  }
+
   scoped_ptr<RenderFrameHostImpl> new_render_frame_host;
   RenderFrameHostImpl* frame_to_announce = NULL;
   int routing_id = MSG_ROUTING_NONE;
@@ -962,7 +960,7 @@ int RenderFrameHostManager::CreateRenderFrame(
   RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
 
   if (proxy) {
-    routing_id = proxy->GetRenderViewHost()->GetRoutingID();
+    routing_id = proxy->GetRoutingID();
     // Delete the existing RenderFrameProxyHost, but reuse the RenderFrameHost.
     // Prevent the process from exiting while we're trying to use it.
     if (!swapped_out) {
@@ -1007,11 +1005,19 @@ int RenderFrameHostManager::CreateRenderFrame(
     }
 
     bool success = InitRenderView(
-        render_view_host, opener_route_id, proxy_routing_id,
-        frame_tree_node_->IsMainFrame());
-    if (success && frame_tree_node_->IsMainFrame()) {
-      // Don't show the main frame's view until we get a DidNavigate from it.
-      render_view_host->GetView()->Hide();
+        render_view_host, opener_route_id, proxy_routing_id, for_main_frame);

[Charlie Reis, 2014/07/28 19:24:28]

Ok, Nick and I looked into this, and I can confirm that for_main_frame and
IsMainFrame are different in the case below.  I'm not 100% sure if it behaves
correctly, but it might be.

During a cross-site iframe navigation, the subframe's
RFHM::UpdateStateForNavigate will call RFHM::CreateRenderFrame on the *root*
RFHM, passing for_main_frame==false.  That's pretty confusing but I can kind of
see how it's "not for a main frame navigation."  The goal is just to create a
swapped out RVH / RFH in the top-level FrameTreeNode.

From here, we pass |for_main_frame| into InitRenderView, which passes it to
WebContents::CreateRenderViewForRenderManager, which creates a
RenderWidgetHostViewChildFrame.  Is that what we want?  Are we supposed to be
creating a RWHVCF for the top-level swapped out RFH/RVH?  (It's *not* what we do
for CreateSwappedOutRenderView, where we call CreateViewForWidget rather than
creating a RWHVCF.  But maybe it's what we want here?)

Assuming that's correct, I still think it's too confusing to have different
for_main_frame and IsMainFrame concepts here.  Let's at least change the name of
the parameter to for_main_frame_navigation, unless you see any better ways to
make it simpler.

[kenrb, 2014/07/28 21:07:21]

As discussed offline, this looks like it is working properly. I changed the name
as you suggest.

+    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 (!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();
     }
@@ -1030,6 +1036,22 @@ int RenderFrameHostManager::CreateRenderFrame(
   return routing_id;
 }
 
+int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
+  // A RenderFrameProxyHost should never be created in the same SiteInstance as
+  // the current RFH.
+  CHECK(instance);
+  CHECK_NE(instance, render_frame_host_->GetSiteInstance());
+
+  RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
+  if (proxy)
+    return proxy->GetRoutingID();
+
+  proxy = new RenderFrameProxyHost(instance, frame_tree_node_);
+  proxy_hosts_[instance->GetId()] = proxy;
+  proxy->InitRenderFrameProxy();
+  return proxy->GetRoutingID();
+}
+
 bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
                                             int opener_route_id,
                                             int proxy_routing_id,
@@ -1058,6 +1080,36 @@ bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
       render_view_host, opener_route_id, proxy_routing_id, for_main_frame);
 }
 
+bool RenderFrameHostManager::InitRenderFrame(
+    RenderFrameHost* render_frame_host) {
+  RenderFrameHostImpl* rfh =
+      static_cast<RenderFrameHostImpl*>(render_frame_host);
+  if (rfh->IsRenderFrameLive())
+    return true;
+
+  int parent_routing_id = MSG_ROUTING_NONE;
+  if (frame_tree_node_->parent()) {
+    parent_routing_id = frame_tree_node_->parent()->render_manager()->
+        GetRoutingIdForSiteInstance(render_frame_host->GetSiteInstance());
+    CHECK_NE(parent_routing_id, MSG_ROUTING_NONE);
+  }
+  return delegate_->CreateRenderFrameForRenderManager(render_frame_host,
+                                                      parent_routing_id);
+}
+
+int RenderFrameHostManager::GetRoutingIdForSiteInstance(
+    SiteInstance* site_instance) {
+  if (render_frame_host_->GetSiteInstance() == site_instance)
+    return render_frame_host_->GetRoutingID();
+
+  RenderFrameProxyHostMap::iterator iter =
+      proxy_hosts_.find(site_instance->GetId());
+  if (iter != proxy_hosts_.end())
+    return iter->second->GetRoutingID();
+
+  return MSG_ROUTING_NONE;
+}
+
 void RenderFrameHostManager::CommitPending() {
   // First check whether we're going to want to focus the location bar after
   // this commit.  We do this now because the navigation hasn't formally
@@ -1175,7 +1227,7 @@ void RenderFrameHostManager::CommitPending() {
   // the RenderFrameHost should be put in the map of RenderFrameHosts pending
   // shutdown. Otherwise, it is stored in the map of proxy hosts.
   if (old_render_frame_host->render_view_host()->rvh_state() ==
-          RenderViewHostImpl::STATE_PENDING_SHUTDOWN) {
+      RenderViewHostImpl::STATE_PENDING_SHUTDOWN) {
     // The proxy for this RenderFrameHost is created when sending the
     // SwapOut message, so check if it already exists and delete it.
     RenderFrameProxyHostMap::iterator iter =
@@ -1192,23 +1244,29 @@ void RenderFrameHostManager::CommitPending() {
           linked_ptr<RenderFrameHostImpl>(old_render_frame_host.release());
     }
   } else {
+    CHECK(proxy_hosts_.find(render_frame_host_->GetSiteInstance()->GetId()) ==
+          proxy_hosts_.end());
+
     // Capture the active view count on the old RFH SiteInstance, since the
-    // ownership will be passed into the proxy and the pointer will be invalid.
+    // ownership might be passed into the proxy and the pointer will be
+    // invalid.
     int active_view_count =
         static_cast<SiteInstanceImpl*>(old_render_frame_host->GetSiteInstance())
             ->active_view_count();
 
-    RenderFrameProxyHostMap::iterator iter =
-        proxy_hosts_.find(old_site_instance_id);
-    CHECK(iter != proxy_hosts_.end());
-    iter->second->TakeFrameHostOwnership(old_render_frame_host.Pass());
+    if (is_main_frame) {
+      RenderFrameProxyHostMap::iterator iter =
+          proxy_hosts_.find(old_site_instance_id);
+      CHECK(iter != proxy_hosts_.end());
+      iter->second->TakeFrameHostOwnership(old_render_frame_host.Pass());
+    }
 
     // If there are no active views in this SiteInstance, it means that
     // this RFH was the last active one in the SiteInstance. Now that we
-    // know that all RFHs are swapped out, we can delete all the RFHs and RVHs
-    // in this SiteInstance.
+    // know that all RFHs are swapped out, we can delete all the RFPHs and
+    // RVHs in this SiteInstance.
     if (!active_view_count) {
-      ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id);
+      ShutdownRenderFrameProxyHostsInSiteInstance(old_site_instance_id);
     } else {
       // If this is a subframe, it should have a CrossProcessFrameConnector
       // created already and we just need to link it to the proper view in the
@@ -1224,7 +1282,7 @@ void RenderFrameHostManager::CommitPending() {
   }
 }
 
-void RenderFrameHostManager::ShutdownRenderFrameHostsInSiteInstance(
+void RenderFrameHostManager::ShutdownRenderFrameProxyHostsInSiteInstance(
     int32 site_instance_id) {
   // First remove any swapped out RFH for this SiteInstance from our own list.
   ClearProxiesInSiteInstance(site_instance_id, frame_tree_node_);
@@ -1308,11 +1366,38 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
     if (new_instance->IsRelatedSiteInstance(current_instance)) {
       opener_route_id =
           delegate_->CreateOpenerRenderViewsForRenderManager(new_instance);
+
+      if (CommandLine::ForCurrentProcess()->HasSwitch(
+              switches::kSitePerProcess)) {
+        // Create the swapped out RVH for the new SiteInstance. This will create

[Charlie Reis, 2014/07/28 19:24:28]

Nick had a good suggestion here.  This first block is awkward to follow because
a subframe RFHM is directly telling the main frame RFHM to do some detailed
operations (creating a RVH/RFH).  It then tells the FrameTree to make sure
there's a proxy in each node, which makes more sense.

It seems preferable for this call site *only* tells the FrameTree to get the
proxies created.  FrameTree::CreateProxiesForSiteInstance should do this block
(lines 1372-1386) internally, such that it creates the RVH if needed before
walking the tree.  That cleans up this call site and seems easier to follow
because we don't have one RFHM directly taking action on another.

[kenrb, 2014/07/28 21:07:20]

Done.

+        // a top-level swapped out RFH as well, which will be wrapped by a
+        // RenderFrameProxyHost in the subsequent call to create proxies.
+        if (!frame_tree_node_->IsMainFrame()) {
+          RenderViewHostImpl* render_view_host =
+              frame_tree_node_->frame_tree()->GetRenderViewHost(new_instance);
+          if (!render_view_host) {
+            frame_tree_node_->frame_tree()->root()->render_manager()
+                ->CreateRenderFrame(new_instance,
+                                    MSG_ROUTING_NONE,
+                                    true,
+                                    false,
+                                    true);
+          }
+        }
+
+        // Ensure that the frame tree has RenderFrameProxyHosts for the new
+        // SiteInstance in all nodes except the current one.
+        frame_tree_node_->frame_tree()->CreateProxiesForSiteInstance(

[Charlie Reis, 2014/07/28 19:24:28]

We should mention that this call creates a scoped_refptr for new_instance, so
it's important to make the call *after* the CreateRenderFrame call.  (Otherwise
the refcount could go to zero before we actually use it.)

[kenrb, 2014/07/28 21:07:21]

This is no longer necessary after moving the above code into
CreateProxiesForSiteInstance, correct?

[Charlie Reis, 2014/07/28 22:32:28]

Acknowledged.

+            frame_tree_node_, new_instance);
+      }
     }
 
     // Create a non-swapped-out pending RFH with the given opener and navigate
     // it.
-    int route_id = CreateRenderFrame(new_instance, opener_route_id, false,
+    int route_id = CreateRenderFrame(new_instance,
+                                     opener_route_id,
+                                     false,
+                                     frame_tree_node_->IsMainFrame(),
                                      delegate_->IsHidden());
     if (route_id == MSG_ROUTING_NONE)
       return NULL;
@@ -1437,7 +1522,8 @@ void RenderFrameHostManager::CancelPending() {
         new RenderFrameProxyHost(site_instance, frame_tree_node_);
     proxy_hosts_[site_instance->GetId()] = proxy;
     pending_render_frame_host->SwapOut(proxy);
-    proxy->TakeFrameHostOwnership(pending_render_frame_host.Pass());
+    if (frame_tree_node_->IsMainFrame())
+      proxy->TakeFrameHostOwnership(pending_render_frame_host.Pass());
   } else {
     // We won't be coming back, so delete this one.
     pending_render_frame_host.reset();
@@ -1478,6 +1564,10 @@ bool RenderFrameHostManager::IsRVHOnSwappedOutList(
       rvh->GetSiteInstance());
   if (!proxy)
     return false;
+  // If there is a proxy without RFH, it is for a subframe in the SiteInstance
+  // of |rvh|. Subframes should be ignored in this case.
+  if (!proxy->render_frame_host())
+    return false;
   return IsOnSwappedOutList(proxy->render_frame_host());
 }