Detach old frame on WebFrame::swap.

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 674 matching lines @@
   if (contains_media_player_)
     render_view_->UnregisterVideoHoleFrame(this);
 #endif
 
   if (!is_subframe_) {
     // RenderFrameProxy is "owned" by RenderFrameImpl in the case it is
     // the main frame. Ensure it is deleted along with this object.
     if (render_frame_proxy_) {
       // The following method calls back into this object and clears
       // |render_frame_proxy_|.
-      render_frame_proxy_->frameDetached();
+      render_frame_proxy_->frameDetached(
+          blink::WebRemoteFrameClient::DetachType::Remove);
     }
 
     // Ensure the RenderView doesn't point to this object, once it is destroyed.
     CHECK_EQ(render_view_->main_render_frame_, this);
     render_view_->main_render_frame_ = nullptr;
   }
 
   render_view_->UnregisterRenderFrame(this);
   g_routing_id_frame_map.Get().erase(routing_id_);
   RenderThread::Get()->RemoveRoute(routing_id_);
@@ skipping 1407 matching lines @@
   // opener after hearing about it from the browser, and the browser does not
   // (yet) care about subframe openers.
   if (is_swapped_out_ || frame->parent())
     return;
 
   // Notify WebContents and all its swapped out RenderViews.
   Send(new FrameHostMsg_DidDisownOpener(routing_id_));
 }
 
 void RenderFrameImpl::frameDetached(blink::WebFrame* frame) {
+  frameDetached(frame, DetachType::Remove);
+}
+
+void RenderFrameImpl::frameDetached(blink::WebFrame* frame, DetachType type) {
   // 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));
 
-  Send(new FrameHostMsg_Detach(routing_id_));
+  if (type == DetachType::Remove)

[Charlie Reis, 2015/06/01 23:41:46]

What other values can it be, and why would this method make sense with them?

I think some comments here would help a lot.  It looks like Swap is the other
value, so maybe it makes sense to mention why we don't send this IPC or call
removeChild when we're doing a swap.

[lfg, 2015/06/02 16:00:01]

I've added some comments, please take a look.

[Charlie Reis, 2015/06/02 17:05:27]

Much better, thanks.

+    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;
 
   // 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);
 
   if (is_subframe_) {
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kSitePerProcess) && render_widget_) {
       render_widget_->UnregisterRenderFrame(this);
     }
-    frame->parent()->removeChild(frame);
+
+    if (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 2713 matching lines @@
 #elif defined(ENABLE_BROWSER_CDMS)
         cdm_manager_,
 #endif
         this);
   }
 
   return cdm_factory_;
 }
 
 }  // namespace content