Fix the RenderFrameProxyHost mop-up logic in RenderFrameHostManager

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 29771b2ab65da73c748a60643a0634acce2ec5a2..0d2a05c94967f91bbc7630f70ce133298465b890 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -68,18 +68,18 @@ RenderFrameHostManager::RenderFrameHostManager(
 }
 
 RenderFrameHostManager::~RenderFrameHostManager() {
-  if (pending_render_frame_host_)
-    UnsetPendingRenderFrameHost();
-
-  if (speculative_render_frame_host_)
-    UnsetSpeculativeRenderFrameHost();
+  if (pending_render_frame_host_) {
+    scoped_ptr<RenderFrameHostImpl> relic = UnsetPendingRenderFrameHost();
+    ShutdownProxiesIfLastActiveFrameInSiteInstance(relic.get());
+  }
 
-  if (render_frame_host_ &&
-      render_frame_host_->GetSiteInstance()->active_frame_count() <= 1U) {
-    ShutdownRenderFrameProxyHostsInSiteInstance(
-        render_frame_host_->GetSiteInstance()->GetId());
+  if (speculative_render_frame_host_) {
+    scoped_ptr<RenderFrameHostImpl> relic = UnsetSpeculativeRenderFrameHost();
+    ShutdownProxiesIfLastActiveFrameInSiteInstance(relic.get());
   }
 
+  ShutdownProxiesIfLastActiveFrameInSiteInstance(render_frame_host_.get());
+
   // Delete any RenderFrameProxyHosts and swapped out RenderFrameHosts.
   // It is important to delete those prior to deleting the current
   // RenderFrameHost, since the CrossProcessFrameConnector (owned by
@@ -604,7 +604,7 @@ void RenderFrameHostManager::SwapOutOldFrame(
   int32 old_site_instance_id =
       old_render_frame_host->GetSiteInstance()->GetId();
   if (!old_render_frame_host->IsRenderFrameLive()) {
-    ShutdownRenderFrameProxyHostsInSiteInstance(old_site_instance_id);
+    ShutdownProxiesIfLastActiveFrameInSiteInstance(old_render_frame_host.get());
     return;
   }
 
@@ -620,8 +620,7 @@ void RenderFrameHostManager::SwapOutOldFrame(
 
     // Also clear out any proxies from this SiteInstance, in case this was the
     // last one keeping other proxies alive.
-    ShutdownRenderFrameProxyHostsInSiteInstance(old_site_instance_id);
-
+    ShutdownProxiesIfLastActiveFrameInSiteInstance(old_render_frame_host.get());
     return;
   }
 
@@ -683,6 +682,7 @@ void RenderFrameHostManager::DiscardUnusedFrame(
       proxy->TakeFrameHostOwnership(render_frame_host.Pass());
   } else {
     // We won't be coming back, so delete this one.
+    ShutdownProxiesIfLastActiveFrameInSiteInstance(render_frame_host.get());
     render_frame_host.reset();
   }
 }
@@ -1754,8 +1754,20 @@ void RenderFrameHostManager::CommitPending() {
         proxy_hosts_.end());
 }
 
-void RenderFrameHostManager::ShutdownRenderFrameProxyHostsInSiteInstance(
-    int32 site_instance_id) {
+void RenderFrameHostManager::ShutdownProxiesIfLastActiveFrameInSiteInstance(
+    RenderFrameHostImpl* render_frame_host) {
+  if (!render_frame_host)
+    return;
+  if (!RenderFrameHostImpl::IsRFHStateActive(render_frame_host->rfh_state()))
+    return;
+  if (render_frame_host->GetSiteInstance()->active_frame_count() > 1U)
+    return;
+
+  // After |render_frame_host| goes away, there will be no active frames left in
+  // its SiteInstance, so we can delete all proxies created in that SiteInstance
+  // on behalf of frames anywhere in the browsing instance.

[Charlie Reis, 2015/04/03 19:55:08]

nit: s/browsing instance/BrowsingInstance/

[ncarter (slow), 2015/04/03 22:03:33]

Done.

+  int32 site_instance_id = render_frame_host->GetSiteInstance()->GetId();
+
   // First remove any swapped out RFH for this SiteInstance from our own list.

[alexmos, 2015/04/03 20:55:55]

While we're here, does this comment need an update?  Seems like this function is
mainly about clearing proxies and only handles swapped-out RFH for main frames.

[ncarter (slow), 2015/04/03 22:03:33]

Done.

But honestly the comment is the least goofy part of this function right now. For
one, the explicit call to ClearProxiesInSiteInstance on |this| seems unnecessary
-- the loop below should either traverse |this| too, unless we're in the middle
of being detached from the tree, in which case our proxies ought to go away when
|this| is deleted.

More puzzlingly, iterating over the RenderWidgetHosts will, I think, cause frame
trees to be traversed multiple times -- the traversal done on behalf of subframe
widgets is unnecessary?

[Charlie Reis, 2015/04/06 16:35:33]

Agreed.  You'd mentioned that it might be worth tracking the active and inactive
frames for a SiteInstance.  Are you still considering that for a followup CL?

   ClearProxiesInSiteInstance(site_instance_id, frame_tree_node_);