Fix the RenderFrameProxyHost mop-up logic in RenderFrameHostManager

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/logging.h"
@@ skipping 50 matching lines @@
       render_frame_delegate_(render_frame_delegate),
       render_view_delegate_(render_view_delegate),
       render_widget_delegate_(render_widget_delegate),
       interstitial_page_(nullptr),
       should_reuse_web_ui_(false),
       weak_factory_(this) {
   DCHECK(frame_tree_node_);
 }
 
 RenderFrameHostManager::~RenderFrameHostManager() {
-  if (pending_render_frame_host_)
-    UnsetPendingRenderFrameHost();
+  if (pending_render_frame_host_) {
+    scoped_ptr<RenderFrameHostImpl> relic = UnsetPendingRenderFrameHost();
+    ShutdownProxiesIfLastActiveFrameInSiteInstance(relic.get());
+  }
 
-  if (speculative_render_frame_host_)
-    UnsetSpeculativeRenderFrameHost();
+  if (speculative_render_frame_host_) {
+    scoped_ptr<RenderFrameHostImpl> relic = UnsetSpeculativeRenderFrameHost();
+    ShutdownProxiesIfLastActiveFrameInSiteInstance(relic.get());
+  }
 
-  if (render_frame_host_ &&
-      render_frame_host_->GetSiteInstance()->active_frame_count() <= 1U) {
-    ShutdownRenderFrameProxyHostsInSiteInstance(
-        render_frame_host_->GetSiteInstance()->GetId());
-  }
+  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
   // RenderFrameProxyHost) points to the RenderWidgetHostView associated with
   // the current RenderFrameHost and uses it during its destructor.
   STLDeleteValues(&proxy_hosts_);
 
   // Release the WebUI prior to resetting the current RenderFrameHost, as the
   // WebUI accesses the RenderFrameHost during cleanup.
@@ skipping 505 matching lines @@
   // Now close any modal dialogs that would prevent us from swapping out.  This
   // must be done separately from SwapOut, so that the PageGroupLoadDeferrer is
   // no longer on the stack when we send the SwapOut message.
   delegate_->CancelModalDialogsForRenderManager();
 
   // If the old RFH is not live, just return as there is no further work to do.
   // It will be deleted and there will be no proxy created.
   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;
   }
 
   // If there are no active frames besides this one, we can delete the old
   // RenderFrameHost once it runs its unload handler, without replacing it with
   // a proxy.
   size_t active_frame_count =
       old_render_frame_host->GetSiteInstance()->active_frame_count();
   if (active_frame_count <= 1) {
     // Tell the old RenderFrameHost to swap out, with no proxy to replace it.
     old_render_frame_host->SwapOut(NULL, true);
     MoveToPendingDeleteHosts(old_render_frame_host.Pass());
 
     // 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;
   }
 
   // Otherwise there are active views and we need a proxy for the old RFH.
   // (There should not be one yet.)
   CHECK(!GetRenderFrameProxyHost(old_render_frame_host->GetSiteInstance()));
   RenderFrameProxyHost* proxy = new RenderFrameProxyHost(
       old_render_frame_host->GetSiteInstance(), frame_tree_node_);
   CHECK(proxy_hosts_.insert(std::make_pair(old_site_instance_id, proxy)).second)
       << "Inserting a duplicate item.";
@@ skipping 41 matching lines @@
 
     // Check if the RenderFrameHost is already swapped out, to avoid swapping it
     // out again.
     if (!render_frame_host->is_swapped_out())
       render_frame_host->SwapOut(proxy, false);
 
     if (frame_tree_node_->IsMainFrame())
       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();
   }
 }
 
 void RenderFrameHostManager::MoveToPendingDeleteHosts(
     scoped_ptr<RenderFrameHostImpl> render_frame_host) {
   // |render_frame_host| will be deleted when its SwapOut ACK is received, or
   // when the timer times out, or when the RFHM itself is deleted (whichever
   // comes first).
   pending_delete_hosts_.push_back(
@@ skipping 1051 matching lines @@
     if (proxy_to_parent)
       proxy_to_parent->SetChildRWHView(render_frame_host_->GetView());
   }
 
   // After all is done, there must never be a proxy in the list which has the
   // same SiteInstance as the current RenderFrameHost.
   CHECK(proxy_hosts_.find(render_frame_host_->GetSiteInstance()->GetId()) ==
         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_);
 
   // Use the safe RenderWidgetHost iterator for now to find all RenderViewHosts
   // in the SiteInstance, then tell their respective FrameTrees to remove all
   // RenderFrameProxyHosts corresponding to them.
   // TODO(creis): Replace this with a RenderFrameHostIterator that protects
   // against use-after-frees if a later element is deleted before getting to it.
   scoped_ptr<RenderWidgetHostIterator> widgets(
       RenderWidgetHostImpl::GetAllRenderWidgetHosts());
   while (RenderWidgetHost* widget = widgets->GetNextHost()) {
@@ skipping 249 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