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 653 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) {

[dcheng, 2015/11/03 04:22:54]

Is it worth DCHECKing our current limitations? For example, this shouldn't be
called on a main frame, right?

[nasko, 2015/11/03 17:59:08]

Done.

[Charlie Reis, 2015/11/04 21:28:30]

I don't see the new DCHECK.  I guess it doesn't apply now that we realized we
need to call it on main frames as well?

+  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 42 matching lines @@
   }
 
   return nullptr;
 }
 
 // RenderFrameImpl ----------------------------------------------------------
 RenderFrameImpl::RenderFrameImpl(const CreateParams& params)
     : frame_(NULL),
       is_main_frame_(true),
       is_local_root_(false),
+      in_browser_initiated_detach_(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 1616 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)
-    frame->parent()->removeChild(frame);
+  // being detached for removal and it is inserted in the set of child frames.
+  // 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) {
+    for (WebFrame* child = frame->parent()->firstChild(); child;

[dcheng, 2015/11/03 04:22:53]

Is there no heuristic we can use inside //content itself? This check makes me
pretty sad.

[nasko, 2015/11/03 17:59:08]

Well, content/ is now in charge of maintaining the frame tree, right? So it is
in content/ that we check whether this frame has been linked into the frame
tree. I could start adding booleans to indicate different states, but I think
this is less prone to problems in the future. Also, it is easy to understand why
we aren't removing the frame from the parent - it isn't in its list of children.

If we have to do this in more than one place, I'm open to exploring storing
state or something else.

+         child = child->nextSibling()) {
+      if (child == frame) {
+        frame->parent()->removeChild(frame);
+        break;
+      }
+    }
+  }
 
   // |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 2884 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