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..c05235557ab687f24074fb4b26c6eb0a896ef852 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -699,6 +699,9 @@ void RenderFrameImpl::CreateFrame(
         replicated_state.scope, WebString::fromUTF8(replicated_state.name),
         replicated_state.sandbox_flags, render_frame,
         previous_sibling_web_frame, frame_owner_properties);
+    // Since the RenderFrame is created and inserted into the frame tree in the
+    // above call to createLocalChild, ensure the state is set properly.

[Charlie Reis, 2015/12/11 00:03:17]

nit: Drop "Since" and "ensure the state is set properly".  The remaining text
justifies setting in_frame_tree_ to true.

Also add a blank line before the comment.

[nasko, 2015/12/11 00:47:25]

Done.

+    render_frame->in_frame_tree_ = true;

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

Charlie: This is newly added code, which was prompted by the comment in
didCommitProvisionalLoad.

[Charlie Reis, 2015/12/11 00:03:17]

Acknowledged.

   } else {
     RenderFrameProxy* proxy =
         RenderFrameProxy::FromRoutingID(proxy_routing_id);
@@ -736,6 +739,13 @@ void RenderFrameImpl::CreateFrame(
 }
 
 // static
+void RenderFrameImpl::DetachFrame(int routing_id) {

[Charlie Reis, 2015/12/11 00:03:17]

Is this ever meant to be called when in_frame_tree_ is true?  Maybe we should
DCHECK if not.

Actually, I just thought of a possible race where that could happen, and it
might get us into trouble.  Suppose the browser process cancels a slow pending
RFH just as it's committing (e.g., due to a cross-process navigation to a
different site).  The renderer process will have committed the RF and set
in_frame_tree_ to true, while the browser process has destroyed the RFH and
won't listen to the commit.

The commit in the renderer process would presumably destroy the
RenderFrameProxy.  Would that mean that we would end up with no proxy when we
should have one?  Yikes.

[nasko, 2015/12/11 00:47:25]

With my latest change it is supposed to. It detaches a frame regardless whether
it is in the tree or not. I'm open to tweaking the name if we have a better one.
With the latest patch version, the RFH destruction will also destroy the RF,
since the IPC is sent from the RFH destructor.
 
Actually, a bit different now - the frame will get completely removed, as the
commit would have happened, the proxy will be gone and if the IPC to destroy the
RenderFrame comes after that, it will totally remove the frame from the tree. :
(

[Charlie Reis, 2015/12/11 00:47:40]

Nick had an idea for this.  If we get here when is_frame_tree_ is true, then the
renderer process has made the RenderFrame active and destroyed the
RenderFrameProxy, while the browser process has destroyed the RenderFrameHost
and assumes the RenderFrameProxyHost still exists.

In this case, could we do a swap back to a RenderFrameProxy here (still
destroying the RenderFrame as requested), perhaps with the same routing ID as
the RenderFrameProxyHost in the browser process?

The one catch is that we might have also sent an IPC from the renderer to the
browser to destroy the RenderFrameProxyHost.  (Is that true?)  Maybe the browser
process can ignore that IPC if it somehow recognizes we're in this case?

[Charlie Reis, 2015/12/11 20:10:11]

This sounds like it's still bad, right?  We could have a RenderFrameProxyHost in
the browser process and no RenderFrameProxy (or RenderFrame) in the renderer
process.  Should we be trying to recreate the RenderFrameProxy?  (Did you want
to address this separately?)

+  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);
 }
@@ -797,6 +807,9 @@ blink::WebFrame* RenderFrameImpl::ResolveOpener(int opener_frame_routing_id,
 RenderFrameImpl::RenderFrameImpl(const CreateParams& params)
     : frame_(NULL),
       is_main_frame_(true),
+      is_local_root_(false),
+      in_browser_initiated_detach_(false),
+      in_frame_tree_(false),
       render_view_(params.render_view->AsWeakPtr()),
       routing_id_(params.routing_id),
       is_swapped_out_(false),
@@ -2438,6 +2451,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 +2483,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 +2507,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 +2942,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