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"
@@ skipping 19 matching lines @@
     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.
+    if (isWebLocalFrame())
+        dispatchUnloadEvent();
     oldFrame->detachChildren();
 
     // If the frame has been detached during detaching its children, return

[dcheng, 2015/06/04 00:58:23]

Update this comment to reflect that we want to check if any JS was executed that
should cause this swap to be cancelled, since it's not just child unload events
anymore.

Also, I have bad news =)

We also need to trigger XHR aborts here... which is done today by calling
FrameLoader::m_documentLoader->detachFromFrame().

[lfg, 2015/06/04 19:16:58]

Updated.
Are you sure we need this? Where should it be?

We already end up calling it, because LocalFrame::detach() calls
FrameLoader::detach() which calls DocumentLoader::detachFromFrame().

[dcheng, 2015/06/05 02:11:50]

We need to call it independently, because it can also cause the frame to be
detached: in that case, we need to cancel the swap().

     // immediately.
     // FIXME: There is no unit test for this condition, so one needs to be
     // written.
     if (!oldFrame->host())
         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(DetachType::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;
         m_parent = nullptr;
     }
 
     if (m_previousSibling) {
         m_previousSibling->m_nextSibling = frame;
-        swap(m_previousSibling, frame->m_previousSibling);
+        std::swap(m_previousSibling, frame->m_previousSibling);

[dcheng, 2015/06/04 00:58:23]

No std:: since swap() is special.

[lfg, 2015/06/04 19:16:58]

Not sure I understand this, do you mean don't use std::swap, or add the "using
std::swap" at the beginning of the function?

[dcheng, 2015/06/05 02:11:50]

Yes, restore the using std::swap and then remove the std:: qualifiers here. This
is the standard idiom for using swap().

     }
     if (m_nextSibling) {
         m_nextSibling->m_previousSibling = frame;
-        swap(m_nextSibling, frame->m_nextSibling);
+        std::swap(m_nextSibling, frame->m_nextSibling);

[dcheng, 2015/06/04 00:58:23]

Ditto.

     }
 
     if (m_opener) {
         m_opener->m_openedFrameTracker->remove(this);
         m_opener->m_openedFrameTracker->add(frame);
-        swap(m_opener, frame->m_opener);
+        std::swap(m_opener, frame->m_opener);

[dcheng, 2015/06/04 00:58:23]

Ditto.

     }
     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(DetachType::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