Fix RenderView reuse issues when canceling a pending RenderFrameHost.

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index f19e585957e80bc0a34959012a09201c46a483fd..c52e552314ecd48c61ed9b9f85265c4f4d791f0e 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -343,10 +343,15 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
 
   // If this RenderFrameHost is swapping with a RenderFrameProxyHost, the
   // RenderFrame will already be deleted in the renderer process. Main frame
-  // RenderFrames will be cleaned up as part of deleting its RenderView. In all
-  // other cases, the RenderFrame should be cleaned up (if it exists).
-  if (is_active() && !frame_tree_node_->IsMainFrame() && render_frame_created_)
+  // RenderFrames will be cleaned up as part of deleting its RenderView if the
+  // RenderView isn't in use by other frames. In all other cases, the
+  // RenderFrame should be cleaned up (if it exists).
+  bool will_render_view_clean_up_render_frame =
+      frame_tree_node_->IsMainFrame() && render_view_host_->ref_count() == 1;

[alexmos, 2016/10/12 20:29:46]

Previously, the FrameMsg_Delete wasn't getting sent when deleting a main frame
and relying on the RVH to clean it up.  But in the case that pending RFH has
other active frames in its SiteInstance, DiscardUnusedFrame is creating a RFPH
in its place that reused that RVH, so the RVH wasn't going away, and we leaked
the main frame RF.  I'm capturing this here by checking if this frame is the
last one holding on to this RVH.  (Checking active_frame_count() == 0 would also
work, I think.)

[nasko, 2016/10/13 21:26:28]

I like the more direct test for it - the ref count on the RVH. It is clear
exactly what it means.

+  if (is_active() && !will_render_view_clean_up_render_frame &&
+      render_frame_created_) {

[nasko, 2016/10/13 21:26:28]

nit: I'd put render_frame_created as the second bool to test, as it makes it a
bit easier to follow.

[alexmos, 2016/10/14 17:07:38]

Done.

     Send(new FrameMsg_Delete(routing_id_));
+  }
 
   // Null out the swapout timer; in crash dumps this member will be null only if
   // the dtor has run.  (It may also be null in tests.)