Detach old frame on WebFrame::swap.

Source/web/WebFrame.cpp

Index: Source/web/WebFrame.cpp
diff --git a/Source/web/WebFrame.cpp b/Source/web/WebFrame.cpp
index b7504104a33a316fa96046dccffa5a74da7d4b35..60da2acf8d3af8f35d5ccc96fb53bd3c08ca0ef0 100644
--- a/Source/web/WebFrame.cpp
+++ b/Source/web/WebFrame.cpp
@@ -37,10 +37,16 @@ Frame* toCoreFrame(const WebFrame* 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().

@@ -50,6 +56,15 @@ bool WebFrame::swap(WebFrame* frame)
     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;
@@ -65,17 +80,17 @@ bool WebFrame::swap(WebFrame* frame)
 
     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);
@@ -86,8 +101,6 @@ bool WebFrame::swap(WebFrame* frame)
     // 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());
@@ -103,7 +116,7 @@ bool WebFrame::swap(WebFrame* frame)
             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());
 
@@ -112,7 +125,7 @@ bool WebFrame::swap(WebFrame* frame)
 
 void WebFrame::detach()
 {
-    toCoreFrame(this)->detach();
+    toCoreFrame(this)->detach(DetachType::Remove);
 }
 
 WebSecurityOrigin WebFrame::securityOrigin() const