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 652 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::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);
 }
 
 // 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 1669 matching lines @@
   // NOTE: This function is called on the frame that is being detached and not
   // 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));
 
+  // 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
   // 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)
+  // 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
   // 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 2886 matching lines @@
   mojo::ServiceProviderPtr service_provider;
   mojo::URLRequestPtr request(mojo::URLRequest::New());
   request->url = mojo::String::From(url);
   mojo_shell_->ConnectToApplication(request.Pass(), GetProxy(&service_provider),
                                     nullptr, nullptr,
                                     base::Bind(&OnGotContentHandlerID));
   return service_provider.Pass();
 }
 
 }  // namespace content