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 623fd1679c52b25ea070f4fb974b3ecbe4f00856..b24888293bd01dd96e6fe2bb7fc6e399e6c0f780 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -670,6 +670,12 @@ void RenderFrameImpl::CreateFrame(
 }
 
 // static
+void RenderFrameImpl::DeleteFrame(int routing_id) {
+  RenderFrameImpl* frame = RenderFrameImpl::FromRoutingID(routing_id);
+  frame->GetWebFrame()->detach();

[Charlie Reis, 2015/10/29 17:28:58]

We should get Daniel to confirm that detach is the right action to take here,
since the frame isn't in the tree yet.

[dcheng, 2015/10/29 18:50:52]

It's kind of wonky, but we should basically treat this as the entry point for
"someone wants to get rid of this frame, so do the appropriate cleanups".

The fact that detach normally removes the frame from the frame tree is mostly
incidental: after all, you can detach the main frame, which doesn't really
remove it from the frame tree.

[nasko, 2015/10/29 19:09:45]

It isn't in the tree, but there are a lot of supporting objects in blink::core
which need to be properly cleaned up. That happens when going through
WebFrame::detach/LocalFrame::detach.
Just deleting the RenderFrame isn't the right behavior, as there are parts of
RenderFrame::frameDetached that are doing cleanup and therefore it must run.

+}
+
+// static
 RenderFrame* RenderFrame::FromWebFrame(blink::WebFrame* web_frame) {
   return RenderFrameImpl::FromWebFrame(web_frame);
 }
@@ -2359,10 +2365,18 @@ void RenderFrameImpl::frameDetached(blink::WebFrame* frame, DetachType type) {
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
                     FrameDetached(frame));
 
+  // When navigating cross-process and a pending RenderFrameHost is used,
+  // the RenderFrame is initialized to replace a RenderFrameProxy and its

[Charlie Reis, 2015/10/29 17:28:58]

This isn't always true, is it?  If the pending RenderFrame is the first in the
process, will it be replacing a proxy?

(We could still end up keeping the process alive in DiscardUnusedFrame in that
case, since another frame might have been added to the process in the meantime.)

[nasko, 2015/10/29 19:09:45]

In the case of pending RenderFrame being first there are two variations:
* Subframe - the RenderView and proxy will be created first, so it isn't really
first in the process.
* Main frame - the RenderView will be creating the RenderFrame and if cancelled,
then the RenderView should be deleted and therefore delete the RenderFrame.

I'll dig a bit more into this though.

+  // routing id is stored until commit time. If the navigation is cancelled,
+  // the RenderFrame must be deleted, but it isn't fully initialized.
+  bool is_fully_initialized = (proxy_routing_id_ == MSG_ROUTING_NONE);
+
+

[Charlie Reis, 2015/10/29 17:28:59]

nit: Remove extra blank line.

[nasko, 2015/10/29 19:09:45]

Done.

   // 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. If the frame is being detached for swap or the frame isn't fully
+  // initialized, we don't need to do this since we are not modifiying the frame
+  // tree.
+  if (is_fully_initialized && type == DetachType::Remove)
     Send(new FrameHostMsg_Detach(routing_id_));
 
   // The |is_detaching_| flag disables Send(). FrameHostMsg_Detach must be
@@ -2387,7 +2401,9 @@ void RenderFrameImpl::frameDetached(blink::WebFrame* frame, DetachType type) {
   // 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)
+  // If the frame isn't fully initialized (e.g. deleting a pending RenderFrame)
+  // it is not linked into the frame tree, so it should not be removed.
+  if (is_fully_initialized && !is_main_frame_ && type == DetachType::Remove)
     frame->parent()->removeChild(frame);
 
   // |frame| is invalid after here.  Be sure to clear frame_ as well, since this