Create RenderFrameProxyHost at time of swap-out instead of commit.

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 13a03b3afc722f900c0028b9db4e88776a54c6ea..2d510e69828e9e6744893e05c91694a4f92a1e3d 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -93,7 +93,13 @@ RenderFrameHostManager::~RenderFrameHostManager() {
   // We should always have a current RenderFrameHost except in some tests.
   render_frame_host_.reset();
 
-  // Delete any swapped out RenderFrameHosts.
+  // Delete any swapped out RenderFrameHosts after sending a message to delete
+  // their counterparts in the renderer process.
+  for (RenderFrameProxyHostMap::iterator iter = proxy_hosts_.begin();
+       iter != proxy_hosts_.end();
+       ++iter) {
+    iter->second->DeleteRendererProxy();
+  }
   STLDeleteValues(&proxy_hosts_);
 }
 
@@ -175,7 +181,8 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     // Note: we don't call InitRenderView here because we are navigating away
     // soon anyway, and we don't have the NavigationEntry for this host.
     delegate_->CreateRenderViewForRenderManager(
-        render_frame_host_->render_view_host(), MSG_ROUTING_NONE, NULL);
+        render_frame_host_->render_view_host(), MSG_ROUTING_NONE,
+        MSG_ROUTING_NONE, NULL);
   }
 
   // If the renderer crashed, then try to create a new one to satisfy this
@@ -185,7 +192,7 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
         dest_render_frame_host->GetSiteInstance());
     if (!InitRenderView(dest_render_frame_host->render_view_host(),
-                        opener_route_id))
+                        opener_route_id, MSG_ROUTING_NONE))
       return NULL;
 
     // Now that we've created a new renderer, be sure to hide it if it isn't
@@ -506,11 +513,17 @@ void RenderFrameHostManager::SwapOutOldPage() {
     }
   }
 
+  // Create the RenderFrameProxyHost that will replace the RenderFrameHost
+  // which is swapping out.
+  RenderFrameProxyHost* proxy = new RenderFrameProxyHost(
+      render_frame_host_->GetSiteInstance(), frame_tree_node_);
+  proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;

[Charlie Reis, 2014/05/15 00:32:50]

To be clear, render_frame_host_ (and its SiteInstance) now exist both as the
current RFH and on the proxy list.  Is that going to violate any assumptions in
the time period before commit?

[ncarter (slow), 2014/05/15 00:42:23]

What guarantees that there's not already something in the map at
proxy_hosts_[id]? It was clear in RenderFrameHostManager::CreateRenderFrame that
this was the case at the time of the assignment, but I get confused why it's not
a problem here.

[nasko, 2014/05/15 18:47:13]

It actually won't exist in two places. Here only a proxy is created, as we can't
have the RFH be in both places at the same time, this will be nasty.
Only at commit time we know that we will no longer use the RFH, so it is at that
point that we pass the ownership of the RFH from RFHM to the proxy.

[nasko, 2014/05/15 18:47:13]

I don't think there is a guarantee and I found one test that hits this case -
CrossSiteTransferTest.ReplaceEntryInProcessThenTranfers. I've changed the code
to ensure it deletes the existing one before creating the new one.

[Charlie Reis, 2014/05/15 22:55:00]

In that case, should IsOnSwappedOutList(RFH) will return false if the proxy is
on the list but the RFH is the current one?  Looks like it will current return
true because it just compares against the proxy's SiteInstance.

Also, what about APIs that look up a host given a SiteInstance, like
GetSwappedOutRenderViewHost and GetRenderFrameProxyHost?  A given SiteInstance
can now be the current RFH and have a proxy on the list.  If we want to hide
that fact, we should probably update those and add a unit test for it.

[nasko, 2014/05/15 23:36:49]

The only way for IsOnSwappedOutList(RFH) to return true is for
(iter->second->render_frame_host() == rfh) to be true. The call to
render_frame_host() will return NULL prior to the proxy taking ownership, which
happens at commit time or when creating swapped out RFH. In both of these cases
it is indeed swapped out. What am I missing?
 
I'm not sure how we would hide the fact that there is a proxy and a frame for
short period of time. I'm open to ideas, but here is my analysis why I think it
is currently ok.

GetSwappedOutRenderViewHost will behave similarly to IsOnSwappedOutList(RFH),
though I had to restore one of my previous changes. See
RFPH::render_view_host().

For GetRenderFrameProxyHost, it is used in 3 locations:
* IsRVHOnSwappedOutList - already safe usage
* GetSwappedOutRenderViewHost - again, safe usage
* CreateRenderFrame - this one is a bit trickier, but there is already a
CHECK_NE ensuring that the current host and the new frame don't have the same
SiteInstance.

[Charlie Reis, 2014/05/16 18:54:07]

I think I confused myself-- I must have been looking at
GetRenderFrameProxyHost(SiteInstance), which won't notice that the RFH is
missing during this time period.  We could detect whether the proxy has a null
RFH and not return it, but I think I agree with your comments below that it's
not necessary to hide it.

+
   // 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
   // time).  When the navigation completes, we will send a message to the
   // ResourceDispatcherHost, allowing the pending RVH's response to resume.
-  render_frame_host_->SwapOut();
+  render_frame_host_->SwapOut(proxy);
 
   // ResourceDispatcherHost has told us to run the onunload handler, which
   // means it is not a download or unsafe page, and we are going to perform the
@@ -558,7 +571,8 @@ bool RenderFrameHostManager::ClearProxiesInSiteInstance(
     // pending shutdown. Otherwise it is deleted.
     if (proxy->render_view_host()->rvh_state() ==
         RenderViewHostImpl::STATE_PENDING_SWAP_OUT) {
-      scoped_ptr<RenderFrameHostImpl> swapped_out_rfh = proxy->PassFrameHost();
+      scoped_ptr<RenderFrameHostImpl> swapped_out_rfh =
+          proxy->PassFrameHostOwnership();
 
       swapped_out_rfh->SetPendingShutdown(base::Bind(
           &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance,
@@ -574,6 +588,7 @@ bool RenderFrameHostManager::ClearProxiesInSiteInstance(
             linked_ptr<RenderFrameHostImpl>(swapped_out_rfh.release());
       }
     } else {
+      proxy->DeleteRendererProxy();
       delete proxy;
     }
     node->render_manager()->proxy_hosts_.erase(site_instance_id);
@@ -898,10 +913,12 @@ int RenderFrameHostManager::CreateRenderFrame(
 
   if (proxy) {
     routing_id = proxy->render_view_host()->GetRoutingID();
+
     // Delete the existing RenderFrameProxyHost, but reuse the RenderFrameHost.
     // Prevent the process from exiting while we're trying to use it.
     if (!swapped_out) {
-      new_render_frame_host = proxy->PassFrameHost();
+      new_render_frame_host = proxy->PassFrameHostOwnership();
+      new_render_frame_host->set_render_frame_proxy(NULL);

[Charlie Reis, 2014/05/15 00:32:50]

Would it make sense to do this step inside PassFrameHostOwnership, or are we
intentionally skipping that in some call sites?

[nasko, 2014/05/15 18:47:13]

Done.

       new_render_frame_host->GetProcess()->AddPendingView();
 
       proxy_hosts_.erase(instance->GetId());
@@ -922,17 +939,24 @@ int RenderFrameHostManager::CreateRenderFrame(
         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_hosts_[instance->GetId()] = new RenderFrameProxyHost(
-          new_render_frame_host.Pass());
+      if (!proxy) {

[Charlie Reis, 2014/05/15 00:32:50]

We already know there's no proxy, since we're in the else block of the if on
line 914.

[nasko, 2014/05/15 18:47:13]

Done.

+        proxy = new RenderFrameProxyHost(
+            new_render_frame_host->GetSiteInstance(), frame_tree_node_);
+      }
+      proxy->TakeFrameHostOwnership(new_render_frame_host.Pass());
+      proxy_routing_id = proxy->GetRoutingID();
+      proxy_hosts_[instance->GetId()] = proxy;
     }
 
-    bool success = InitRenderView(render_view_host, opener_route_id);
+    bool success = InitRenderView(
+        render_view_host, opener_route_id, proxy_routing_id);
     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();
@@ -950,7 +974,8 @@ int RenderFrameHostManager::CreateRenderFrame(
 }
 
 bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
-                                            int opener_route_id) {
+                                            int opener_route_id,
+                                            int proxy_routing_id) {
   // We may have initialized this RenderViewHost for another RenderFrameHost.
   if (render_view_host->IsRenderViewLive())
     return true;
@@ -972,7 +997,8 @@ bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
   }
 
   return delegate_->CreateRenderViewForRenderManager(
-      render_view_host, opener_route_id, cross_process_frame_connector_);
+      render_view_host, opener_route_id, proxy_routing_id,
+      cross_process_frame_connector_);
 }
 
 void RenderFrameHostManager::CommitPending() {
@@ -1093,24 +1119,21 @@ void RenderFrameHostManager::CommitPending() {
     DCHECK(old_render_frame_host->is_swapped_out() ||
            !RenderViewHostImpl::IsRVHStateActive(
                old_render_frame_host->render_view_host()->rvh_state()));
-  // Temp fix for http://crbug.com/90867 until we do a better cleanup to make
-  // sure we don't get different rvh instances for the same site instance
-  // in the same rvhmgr.
-  // TODO(creis): Clean this up.
-  RenderFrameProxyHostMap::iterator iter =
-      proxy_hosts_.find(old_site_instance_id);
-  if (iter != proxy_hosts_.end() &&
-      iter->second->render_frame_host() != old_render_frame_host) {
-    // Delete the proxy that will be replaced in the map to avoid a leak.
-    delete iter->second;
-  }
 
   // If the RenderViewHost backing the RenderFrameHost is pending shutdown,
   // 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) {
-    proxy_hosts_.erase(old_site_instance_id);
+    // The proxy for this RenderFrameHost is created when sending the
+    // SwapOut message, so check if it already exists and delete it.
+    RenderFrameProxyHostMap::iterator iter =
+      proxy_hosts_.find(old_site_instance_id);

[Charlie Reis, 2014/05/15 00:32:50]

nit: Wrong indent.

[nasko, 2014/05/15 18:47:13]

Done.

+    if (iter != proxy_hosts_.end()) {
+      iter->second->DeleteRendererProxy();
+      delete iter->second;
+      proxy_hosts_.erase(iter);
+    }
     RFHPendingDeleteMap::iterator pending_delete_iter =
         pending_delete_hosts_.find(old_site_instance_id);
     if (pending_delete_iter == pending_delete_hosts_.end() ||
@@ -1119,18 +1142,22 @@ void RenderFrameHostManager::CommitPending() {
           linked_ptr<RenderFrameHostImpl>(old_render_frame_host.release());
     }
   } else {
+    // 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.
+    int active_view_count = static_cast<SiteInstanceImpl*>(
+            old_render_frame_host->GetSiteInstance())->active_view_count();

[Charlie Reis, 2014/05/15 00:32:50]

nit: Wrong indent.

[nasko, 2014/05/15 18:47:13]

Done.

+
+    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.  We do this after ensuring the RFH is on the
-    // swapped out list to simplify the deletion.
-    if (!static_cast<SiteInstanceImpl*>(
-            old_render_frame_host->GetSiteInstance())->active_view_count()) {
-      old_render_frame_host.reset();
+    // in this SiteInstance.
+    if (!active_view_count) {

[Charlie Reis, 2014/05/15 00:32:50]

nit: No braces needed.

[nasko, 2014/05/15 18:47:13]

Done.

       ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id);
-    } else {
-      proxy_hosts_[old_site_instance_id] = new RenderFrameProxyHost(
-          old_render_frame_host.Pass());
     }
   }
 }
@@ -1343,10 +1370,11 @@ void RenderFrameHostManager::CancelPending() {
     // Any currently suspended navigations are no longer needed.
     pending_render_frame_host->render_view_host()->CancelSuspendedNavigations();
 
-    pending_render_frame_host->SwapOut();
-
-    proxy_hosts_[site_instance->GetId()] = new RenderFrameProxyHost(
-        pending_render_frame_host.Pass());
+    RenderFrameProxyHost* proxy =
+        new RenderFrameProxyHost(site_instance, frame_tree_node_);
+    pending_render_frame_host->SwapOut(proxy);
+    proxy->TakeFrameHostOwnership(pending_render_frame_host.Pass());
+    proxy_hosts_[site_instance->GetId()] = proxy;

[Charlie Reis, 2014/05/15 00:32:50]

I'm a little uncomfortable that we put the proxy into proxy_hosts_ before
calling SwapOut in one case and after calling it in the other.  SwapOut
synchronously calls back into SwappedOut, which might call RequestTransferURL,
so I think we should be consistent about the state of proxy_hosts_ during this
call if possible.

[nasko, 2014/05/15 18:47:13]

This was just a honest omission, not intentional. I've made all creations for
RFPH to use the same sequence.

   } else {
     // We won't be coming back, so delete this one.
     pending_render_frame_host.reset();