Fix leaking of RenderFrames.

content/renderer/render_frame_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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 681 matching lines @@
     if (previous_sibling_proxy)
       previous_sibling_web_frame = previous_sibling_proxy->web_frame();
 
     // Create the RenderFrame and WebLocalFrame, linking the two.
     render_frame =
         RenderFrameImpl::Create(parent_proxy->render_view(), routing_id);
     web_frame = parent_web_frame->createLocalChild(
         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);
     CHECK(proxy);
     render_frame = RenderFrameImpl::Create(proxy->render_view(), routing_id);
     render_frame->proxy_routing_id_ = proxy_routing_id;
     web_frame = blink::WebLocalFrame::createProvisional(
         render_frame, proxy->web_frame(), replicated_state.sandbox_flags,
         frame_owner_properties);
   }
@@ skipping 17 matching lines @@
       // WasShown and WasHidden, separating page-level visibility from
       // frame-level visibility.
       render_frame->render_widget_->RegisterRenderFrame(render_frame);
     }
   }
 
   render_frame->Initialize();
 }
 
 // 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);
 }
 
 // static
 RenderFrameImpl* RenderFrameImpl::FromWebFrame(blink::WebFrame* web_frame) {
   FrameMap::iterator iter = g_frame_map.Get().find(web_frame);
   if (iter != g_frame_map.Get().end())
     return iter->second;
   return NULL;
@@ skipping 41 matching lines @@
     return opener_frame->GetWebFrame();
   }
 
   return nullptr;
 }
 
 // RenderFrameImpl ----------------------------------------------------------
 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),
       render_frame_proxy_(NULL),
       is_detaching_(false),
       proxy_routing_id_(MSG_ROUTING_NONE),
 #if defined(ENABLE_PLUGINS)
       plugin_power_saver_helper_(nullptr),
 #endif
       cookie_jar_(this),
@@ skipping 1621 matching lines @@
 
   // Create the RenderFrame and WebLocalFrame, linking the two.
   RenderFrameImpl* child_render_frame = RenderFrameImpl::Create(
       render_view_.get(), child_routing_id);
   blink::WebLocalFrame* web_frame =
       WebLocalFrame::create(scope, child_render_frame);
   child_render_frame->SetWebFrame(web_frame);
 
   // 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;
 }
 
 void RenderFrameImpl::didChangeOpener(blink::WebFrame* opener) {
   // Only active frames are able to disown their opener.
   if (!opener && is_swapped_out_)
     return;
 
@@ skipping 11 matching lines @@
   // the parent frame.  This is different from createChildFrame() which is
   // called on the parent frame.
   CHECK(!is_detaching_);
   DCHECK(!frame_ || frame_ == frame);
 
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, FrameDetached());
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
                     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
   // sent before setting |is_detaching_| to true.
   is_detaching_ = true;
 
   // Clean up the associated RenderWidget for the frame, if there is one.
   if (render_widget_) {
     render_widget_->UnregisterRenderFrame(this);
     render_widget_->CloseForFrame();
   }
 
   // We need to clean up subframes by removing them from the map and deleting
   // the RenderFrameImpl.  In contrast, the main frame is owned by its
   // containing RenderViewHost (so that they have the same lifetime), so only
   // removal from the map is needed and no deletion.
   FrameMap::iterator it = g_frame_map.Get().find(frame);
   CHECK(it != g_frame_map.Get().end());
   CHECK_EQ(it->second, this);
   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
   // it.
   frame->close();
   frame_ = nullptr;
 
   delete this;
   // Object is invalid after this point.
 }
@@ skipping 408 matching lines @@
   WebURLResponseExtraDataImpl* extra_data = GetExtraDataFromResponse(
       frame->dataSource()->response());
   is_using_lofi_ = extra_data && extra_data->is_using_lofi();
 
   if (proxy_routing_id_ != MSG_ROUTING_NONE) {
     RenderFrameProxy* proxy =
         RenderFrameProxy::FromRoutingID(proxy_routing_id_);
     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
     // and ensure RenderWidget is no longer in swapped out mode.
     if (is_main_frame_) {
       CHECK(!render_view_->main_render_frame_);
       render_view_->main_render_frame_ = this;
       if (render_view_->is_swapped_out())
         render_view_->SetSwappedOut(false);
     }
@@ skipping 2577 matching lines @@
       media::ConvertToSwitchOutputDeviceCB(web_callbacks);
   scoped_refptr<media::AudioOutputDevice> device =
       AudioDeviceFactory::NewOutputDevice(routing_id_, 0, sink_id.utf8(),
                                           security_origin);
   media::OutputDeviceStatus status = device->GetDeviceStatus();
   device->Stop();
   callback.Run(status);
 }
 
 }  // namespace content