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);
+
+    // 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);
     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 78 matching lines @@
     return opener_frame->GetWebFrame();
   }
 
   return nullptr;
 }
 
 // RenderFrameImpl ----------------------------------------------------------
 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),
       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 383 matching lines @@
   while ((observer = it.GetNext()) != NULL) {
     if (observer->OnMessageReceived(msg))
       return true;
   }
 
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(RenderFrameImpl, 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,
                         OnCustomContextMenuAction)
     IPC_MESSAGE_HANDLER(InputMsg_Undo, OnUndo)
     IPC_MESSAGE_HANDLER(InputMsg_Redo, OnRedo)
     IPC_MESSAGE_HANDLER(InputMsg_Cut, OnCut)
     IPC_MESSAGE_HANDLER(InputMsg_Copy, OnCopy)
     IPC_MESSAGE_HANDLER(InputMsg_Paste, OnPaste)
     IPC_MESSAGE_HANDLER(InputMsg_PasteAndMatchStyle, OnPasteAndMatchStyle)
@@ skipping 221 matching lines @@
     proxy->SetReplicatedState(replicated_frame_state);
 
   // Safe to exit if no one else is using the process.
   // TODO(nasko): Remove the dependency on RenderViewImpl here and ref count
   // the process based on the lifetime of this RenderFrameImpl object.
   if (is_main_frame) {
     render_view->WasSwappedOut();
   }
 }
 
+void RenderFrameImpl::OnDeleteFrame() {
+  // TODO(nasko): If this message is received right after a commit has
+  // swapped a RenderFrameProxy with RenderFrame, the proxy need to be

[Charlie Reis, 2015/12/14 22:12:52]

nit: with this
nit: needs

[nasko, 2015/12/14 22:27:59]

Done.

+  // recreated instead of the frame being deleted.

[Charlie Reis, 2015/12/14 22:12:52]

Don't we need to delete the frame as well as recreate the proxy?

[nasko, 2015/12/14 22:27:59]

Done.

+  // See https://crbug.com/569683 for details.
+  in_browser_initiated_detach_ = true;
+
+  // This will result in a call to RendeFrameImpl::frameDetached, which
+  // deletes the object. Do not access |this| after detach.
+  frame_->detach();
+}
+
 void RenderFrameImpl::OnContextMenuClosed(
     const CustomContextMenuContext& custom_context) {
   if (custom_context.request_id) {
     // External request, should be in our map.
     ContextMenuClient* client =
         pending_context_menus_.Lookup(custom_context.request_id);
     if (client) {
       client->OnMenuClosed(custom_context.request_id);
       pending_context_menus_.Remove(custom_context.request_id);
     }
@@ skipping 977 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