Detach old frame on WebFrame::swap.

Source/web/WebFrame.cpp

 // Copyright 2014 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 "config.h"
 #include "public/web/WebFrame.h"
 
 #include "bindings/core/v8/WindowProxyManager.h"
 #include "core/frame/FrameHost.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/RemoteFrame.h"
 #include "core/html/HTMLFrameElementBase.h"
 #include "core/html/HTMLFrameOwnerElement.h"
+#include "core/loader/DocumentLoader.h"
 #include "core/page/Page.h"
 #include "platform/UserGestureIndicator.h"
 #include "platform/heap/Handle.h"
 #include "public/web/WebElement.h"
 #include "public/web/WebSandboxFlags.h"
 #include "web/OpenedFrameTracker.h"
 #include "web/RemoteBridgeFrameOwner.h"
 #include "web/WebLocalFrameImpl.h"
 #include "web/WebRemoteFrameImpl.h"
 #include <algorithm>
 
 namespace blink {
 
 Frame* toCoreFrame(const WebFrame* frame)
 {
     if (!frame)
         return 0;
 
     return frame->isWebLocalFrame()
         ? static_cast<Frame*>(toWebLocalFrameImpl(frame)->frame())
         : toWebRemoteFrameImpl(frame)->frame();
 }
 
 bool WebFrame::swap(WebFrame* frame)
 {
     using std::swap;
     RefPtrWillBeRawPtr<Frame> oldFrame = toCoreFrame(this);
+#if !ENABLE(OILPAN)
+    RefPtrWillBeRawPtr<WebLocalFrameImpl> protectWebLocalFrame = isWebLocalFrame() ? toWebLocalFrameImpl(this) : nullptr;
+    RefPtrWillBeRawPtr<WebRemoteFrameImpl> protectWebRemoteFrame = isWebRemoteFrame() ? toWebRemoteFrameImpl(this) : nullptr;
+#endif
 
-    // All child frames must be detached first.
-    oldFrame->detachChildren();
+    bool readyToSwap = true;
+    if (isWebLocalFrame())
+        readyToSwap = toLocalFrame(toCoreFrame(this))->loader().prepareForCommit();
+    else
+        oldFrame->detachChildren();

[dcheng, 2015/06/08 19:34:42]

If we're swapping remote to local, and detaching the children causes the remote
frame to be detached, we also need to bail out.

[lfg, 2015/06/10 18:41:28]

At first I thought this would never be the case, but it can happen if you have
an A -> B -> A situation.

 
-    // If the frame has been detached during detaching its children, return
-    // immediately.
+    // If the frame has been detached during detaching by any JS code, we should
+    // cancel the swap.
     // FIXME: There is no unit test for this condition, so one needs to be
     // written.
-    if (!oldFrame->host())
+    if (!readyToSwap)

[dcheng, 2015/06/08 19:34:42]

I think you can keep the original check here, since that will handle both the
local and remote cases.

[lfg, 2015/06/10 18:41:28]

Done.

         return false;
 
+    FrameOwner* owner = oldFrame->owner();
+    FrameHost* host = oldFrame->host();
+
+    // Frame::detach will call clearForClose(), we need to call
+    // clearForNavigate() before detaching.
+    oldFrame->prepareSwapFrom(oldFrame.get());
+
+    oldFrame->detach(FrameDetachType::Swap);
+
     if (m_parent) {
         if (m_parent->m_firstChild == this)
             m_parent->m_firstChild = frame;
         if (m_parent->m_lastChild == this)
             m_parent->m_lastChild = frame;
         // FIXME: This is due to the fact that the |frame| may be a provisional
         // local frame, because we don't know if the navigation will result in
         // an actual page or something else, like a download. The PlzNavigate
         // project will remove the need for provisional local frames.
         frame->m_parent = m_parent;
@@ skipping 16 matching lines @@
     }
     if (!m_openedFrameTracker->isEmpty()) {
         m_openedFrameTracker->updateOpener(frame);
         frame->m_openedFrameTracker.reset(m_openedFrameTracker.release());
     }
 
     // Finally, clone the state of the current Frame into one matching
     // the type of the passed in WebFrame.
     // FIXME: This is a bit clunky; this results in pointless decrements and
     // increments of connected subframes.
-    FrameOwner* owner = oldFrame->owner();
-    oldFrame->disconnectOwnerElement();
     if (frame->isWebLocalFrame()) {
         LocalFrame& localFrame = *toWebLocalFrameImpl(frame)->frame();
         ASSERT(owner == localFrame.owner());
         if (owner) {
             if (owner->isLocal()) {
                 HTMLFrameOwnerElement* ownerElement = toHTMLFrameOwnerElement(owner);
                 ownerElement->setContentFrame(localFrame);
                 ownerElement->setWidget(localFrame.view());
             } else {
                 toRemoteBridgeFrameOwner(owner)->setContentFrame(toWebLocalFrameImpl(frame));
             }
         } else {
             localFrame.page()->setMainFrame(&localFrame);
         }
     } else {
-        toWebRemoteFrameImpl(frame)->initializeCoreFrame(oldFrame->host(), owner, oldFrame->tree().name());
+        toWebRemoteFrameImpl(frame)->initializeCoreFrame(host, owner, oldFrame->tree().name());
     }
     toCoreFrame(frame)->finishSwapFrom(oldFrame.get());
 
     return true;
 }
 
 void WebFrame::detach()
 {
-    toCoreFrame(this)->detach();
+    toCoreFrame(this)->detach(FrameDetachType::Remove);
 }
 
 WebSecurityOrigin WebFrame::securityOrigin() const
 {
     return WebSecurityOrigin(toCoreFrame(this)->securityContext()->securityOrigin());
 }
 
 
 void WebFrame::setFrameOwnerSandboxFlags(WebSandboxFlags flags)
 {
@@ skipping 217 matching lines @@
     void WebFrame::traceFrames(VisitorDispatcher visitor, WebFrame* frame) { traceFramesImpl(visitor, frame); }                \
     void WebFrame::clearWeakFrames(VisitorDispatcher visitor) { clearWeakFramesImpl(visitor); }
 
 DEFINE_VISITOR_METHOD(Visitor*)
 DEFINE_VISITOR_METHOD(InlinedGlobalMarkingVisitor)
 
 #undef DEFINE_VISITOR_METHOD
 #endif
 
 } // namespace blink