Fix RenderView reuse issues when canceling a pending RenderFrameHost.

content/browser/frame_host/render_frame_host_impl.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_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 325 matching lines @@
   if (delegate_ && render_frame_created_)
     delegate_->RenderFrameDeleted(this);
 
   // If this was the last active frame in the SiteInstance, the
   // DecrementActiveFrameCount call will trigger the deletion of the
   // SiteInstance's proxies.
   GetSiteInstance()->DecrementActiveFrameCount();
 
   // 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.)
   swapout_event_monitor_timeout_.reset();
 
   for (const auto& iter: visual_state_callbacks_) {
     iter.second.Run(false);
   }
 
   if (render_widget_host_ &&
@@ skipping 2792 matching lines @@
   web_bluetooth_service_->SetClientConnectionErrorHandler(base::Bind(
       &RenderFrameHostImpl::DeleteWebBluetoothService, base::Unretained(this)));
   return web_bluetooth_service_.get();
 }
 
 void RenderFrameHostImpl::DeleteWebBluetoothService() {
   web_bluetooth_service_.reset();
 }
 
 }  // namespace content