Fix leaking of RenderFrames.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 1693d1f77ba56e0899765c4c384cf8835c0c3484..f322f51c9f31158cbb5853228181694a9b6abb88 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -671,6 +671,13 @@ void RenderFrameImpl::CreateFrame(
 }
 
 // static
+void RenderFrameImpl::DetachFrame(int routing_id) {
+  RenderFrameImpl* frame = RenderFrameImpl::FromRoutingID(routing_id);
+  frame->set_in_browser_initiated_detach();
+  frame->GetWebFrame()->detach();
+}
+
+// static
 RenderFrame* RenderFrame::FromWebFrame(blink::WebFrame* web_frame) {
   return RenderFrameImpl::FromWebFrame(web_frame);
 }
@@ -733,6 +740,8 @@ RenderFrameImpl::RenderFrameImpl(const CreateParams& params)
     : frame_(NULL),
       is_main_frame_(true),
       is_local_root_(false),
+      in_browser_initiated_detach_(false),
+      is_inserted_in_frame_tree_(false),
       render_view_(params.render_view->AsWeakPtr()),
       routing_id_(params.routing_id),
       is_swapped_out_(false),
@@ -2338,6 +2347,7 @@ blink::WebFrame* RenderFrameImpl::createChildFrame(
 
   // Add the frame to the frame tree and initialize it.
   parent->appendChild(web_frame);
+  child_render_frame->is_inserted_in_frame_tree_ = true;
   child_render_frame->Initialize();
 
   return web_frame;
@@ -2369,9 +2379,8 @@ void RenderFrameImpl::frameDetached(blink::WebFrame* frame, DetachType type) {
                     FrameDetached(frame));
 
   // We only notify the browser process when the frame is being detached for
-  // removal. If the frame is being detached for swap, we don't need to do this
-  // since we are not modifiying the frame tree.
-  if (type == DetachType::Remove)
+  // removal and it was initiated from the renderer process.
+  if (!in_browser_initiated_detach_ && type == DetachType::Remove)
     Send(new FrameHostMsg_Detach(routing_id_));
 
   // The |is_detaching_| flag disables Send(). FrameHostMsg_Detach must be
@@ -2394,10 +2403,13 @@ void RenderFrameImpl::frameDetached(blink::WebFrame* frame, DetachType type) {
   g_frame_map.Get().erase(it);
 
   // Only remove the frame from the renderer's frame tree if the frame is
-  // being detached for removal. In the case of a swap, the frame needs to
-  // remain in the tree so WebFrame::swap() can replace it with the new frame.
-  if (!is_main_frame_ && type == DetachType::Remove)
+  // being detached for removal and is already inserted in the frame tree.
+  // In the case of a swap, the frame needs to remain in the tree so
+  // WebFrame::swap() can replace it with the new frame.
+  if (!is_main_frame_ && is_inserted_in_frame_tree_ &&
+      type == DetachType::Remove) {
     frame->parent()->removeChild(frame);
+  }
 
   // |frame| is invalid after here.  Be sure to clear frame_ as well, since this
   // object may not be deleted immediately and other methods may try to access
@@ -2802,6 +2814,7 @@ void RenderFrameImpl::didCommitProvisionalLoad(
     CHECK(proxy);
     proxy->web_frame()->swap(frame_);
     proxy_routing_id_ = MSG_ROUTING_NONE;
+    is_inserted_in_frame_tree_ = true;

[Charlie Reis, 2015/11/04 21:28:30]

Is it correct to put this behind a proxy_routing_id_ check?  I would think that
a pending RFH in a new process wouldn't usually have a proxy for its own frame.

Maybe we put the RFH into the frame tree up front in that case (since a swap
apparently isn't required)?  Does this bool get set to true in that case?

Sorry if I'm misunderstanding.

[nasko, 2015/12/10 21:26:52]

Unless it navigates back to its parent's frame process :).

I don't think of this in terms of presence of proxy routing id. We are doing a
frame swap and replacing some frame in the tree with this frame. As such, from
this point on this frame is in the tree.
 
When we aren't replacing a proxy, I think we directly add it into the frame
tree. I've made sure we set this to true there.
 
No worries, this is complicated and it is good to be paranoid about all edge
cases.

 
     // If this is the main frame going from a remote frame to a local frame,
     // it needs to set RenderViewImpl's pointer for the main frame to itself