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 9e432ba9394f11a708bce49a6d40d9c62c5f3a65..d44ea50cb7defaaaccb22932c293c4cdfbae7b78 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -699,6 +699,10 @@ void RenderFrameImpl::CreateFrame(
         replicated_state.scope, WebString::fromUTF8(replicated_state.name),
         replicated_state.sandbox_flags, render_frame,
         previous_sibling_web_frame, frame_owner_properties);
+
+    // The RenderFrame is created and inserted into the frame tree in the above
+    // call to createLocalChild.
+    render_frame->in_frame_tree_ = true;
   } else {
     RenderFrameProxy* proxy =
         RenderFrameProxy::FromRoutingID(proxy_routing_id);
@@ -797,6 +801,8 @@ blink::WebFrame* RenderFrameImpl::ResolveOpener(int opener_frame_routing_id,
 RenderFrameImpl::RenderFrameImpl(const CreateParams& params)
     : frame_(NULL),
       is_main_frame_(true),
+      in_browser_initiated_detach_(false),
+      in_frame_tree_(false),
       render_view_(params.render_view->AsWeakPtr()),
       routing_id_(params.routing_id),
       is_swapped_out_(false),
@@ -1200,6 +1206,7 @@ bool RenderFrameImpl::OnMessageReceived(const IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(FrameMsg_Navigate, OnNavigate)
     IPC_MESSAGE_HANDLER(FrameMsg_BeforeUnload, OnBeforeUnload)
     IPC_MESSAGE_HANDLER(FrameMsg_SwapOut, OnSwapOut)
+    IPC_MESSAGE_HANDLER(FrameMsg_Delete, OnDeleteFrame)
     IPC_MESSAGE_HANDLER(FrameMsg_Stop, OnStop)
     IPC_MESSAGE_HANDLER(FrameMsg_ContextMenuClosed, OnContextMenuClosed)
     IPC_MESSAGE_HANDLER(FrameMsg_CustomContextMenuAction,
@@ -1441,6 +1448,11 @@ void RenderFrameImpl::OnSwapOut(
   }
 }
 
+void RenderFrameImpl::OnDeleteFrame() {

[Charlie Reis, 2015/12/14 21:22:39]

We're putting off the cancel/commit race bug for later, right?  Can you file a
bug for that one and optionally mention it in a TODO here?  (If we receive this
after we have just committed, we probably need to resurrect the proxy we
destroyed.)

[nasko, 2015/12/14 22:08:29]

Done.

+  in_browser_initiated_detach_ = true;
+  frame_->detach();

[Charlie Reis, 2015/12/14 21:22:39]

Sanity check: We aren't creating UaFs by doing this, after this method returns?

Let's at least add a comment saying that this will trigger a call to
frameDetached and thus |this| will be deleted.

[nasko, 2015/12/14 22:08:29]

Done.

+}
+
 void RenderFrameImpl::OnContextMenuClosed(
     const CustomContextMenuContext& custom_context) {
   if (custom_context.request_id) {
@@ -2438,6 +2450,7 @@ blink::WebFrame* RenderFrameImpl::createChildFrame(
 
   // Add the frame to the frame tree and initialize it.
   parent->appendChild(web_frame);
+  child_render_frame->in_frame_tree_ = true;
   child_render_frame->Initialize();
 
   return web_frame;
@@ -2469,9 +2482,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
@@ -2494,10 +2506,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_ && 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
@@ -2926,6 +2941,7 @@ void RenderFrameImpl::didCommitProvisionalLoad(
     CHECK(proxy);
     proxy->web_frame()->swap(frame_);
     proxy_routing_id_ = MSG_ROUTING_NONE;
+    in_frame_tree_ = true;
 
     // 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