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 16001eed4b88c18746c22fdd068b86c790f65549..b9bd83fc9f886865cf64cd41fbce8fed5141a433 100644
--- a/Source/web/WebFrame.cpp
+++ b/Source/web/WebFrame.cpp
@@ -35,11 +35,10 @@ Frame* toCoreFrame(const WebFrame* frame)
 
 bool WebFrame::swap(WebFrame* frame)
 {
-    using std::swap;
     RefPtrWillBeRawPtr<Frame> oldFrame = toCoreFrame(this);
 
-    // All child frames must be detached first.
-    oldFrame->detachChildren();
+    if (isWebLocalFrame())

[dcheng, 2015/03/27 04:08:47]

I think you can get the unload() behavior you want for free by doing something
like:

dispatchUnloadEvent();  // Only dispatches unload event for this subframe.
oldFrame->detachChildren();  // Now any remaining children are detached.

// If the unload event itself or the children already detached this frame, then
just
// return. There is no longer a place to swap this frame into the frame tree.
if (!oldFrame->host())
  return;

/* cache state */
oldFrame->detach();  // The children of the original frame are already gone, but
detach oldFrame itself now.

[lfg, 2015/03/27 04:57:52]

Interesting, I'll try that.

+        dispatchUnloadEvent();
 
     // If the frame has been detached during detaching its children, return
     // immediately.
@@ -48,44 +47,54 @@ bool WebFrame::swap(WebFrame* frame)
     if (!oldFrame->host())
         return false;
 
-    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;
+    FrameOwner* owner = oldFrame->owner();
+    const AtomicString name = oldFrame->tree().name();
+    FrameHost* host = oldFrame->host();
+    WebFrame* parent = m_parent;
+    WebFrame* parentOldFirstChild = parent ? parent->m_firstChild : nullptr;
+    WebFrame* parentOldLastChild = parent ? parent->m_lastChild : nullptr;
+    WebFrame* previousSibling = m_previousSibling;
+    WebFrame* nextSibling = m_nextSibling;
+    WebFrame* opener = m_opener;
+    WebPrivateOwnPtr<OpenedFrameTracker> openedFrameTracker;
+    openedFrameTracker.reset(m_openedFrameTracker.release());
+
+    oldFrame->prepareSwapFrom(oldFrame.get());
+
+    oldFrame->detach();
+    // Object is invalid beyond this point.

[dcheng, 2015/03/27 04:08:47]

I don't think this comment is completely accurate--after all, we call
finishSwapFrom(oldFrame.get()) below.

[lfg, 2015/03/27 04:57:52]

The WebFrame is gone (i.e. the object pointed by the 'this' pointer), but the
Frame is still alive because of the refcounted reference we took at the
beginning of this function.

+
+    if (parent) {
+        if (parentOldFirstChild == this)
+            parent->m_firstChild = frame;
+        if (parentOldLastChild == this)
+            parent->m_lastChild = frame;
+        frame->m_parent = parent;
     }
 
-    if (m_previousSibling) {
-        m_previousSibling->m_nextSibling = frame;
-        swap(m_previousSibling, frame->m_previousSibling);
+    if (previousSibling) {
+        previousSibling->m_nextSibling = frame;
+        frame->m_previousSibling = previousSibling;
     }
-    if (m_nextSibling) {
-        m_nextSibling->m_previousSibling = frame;
-        swap(m_nextSibling, frame->m_nextSibling);
+    if (nextSibling) {
+        nextSibling->m_previousSibling = frame;
+        frame->m_nextSibling = nextSibling;
     }
 
-    if (m_opener) {
-        m_opener->m_openedFrameTracker->remove(this);
-        m_opener->m_openedFrameTracker->add(frame);
-        swap(m_opener, frame->m_opener);
+    if (opener) {
+        opener->m_openedFrameTracker->remove(this);
+        opener->m_openedFrameTracker->add(frame);
+        frame->m_opener = opener;
     }
-    if (!m_openedFrameTracker->isEmpty()) {
-        m_openedFrameTracker->updateOpener(frame);
-        frame->m_openedFrameTracker.reset(m_openedFrameTracker.release());
+    if (!openedFrameTracker->isEmpty()) {
+        openedFrameTracker->updateOpener(frame);
+        frame->m_openedFrameTracker.reset(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());
@@ -101,9 +110,12 @@ bool WebFrame::swap(WebFrame* frame)
             localFrame.page()->setMainFrame(&localFrame);
         }
     } else {
-        toWebRemoteFrameImpl(frame)->initializeCoreFrame(oldFrame->host(), owner, oldFrame->tree().name());
+        toWebRemoteFrameImpl(frame)->initializeCoreFrame(host, owner, name);
     }
     toCoreFrame(frame)->finishSwapFrom(oldFrame.get());
+    toCoreFrame(frame)->tree().invalidateScopedChildCount();
+    toCoreFrame(frame)->host()->incrementSubframeCount();
+    openedFrameTracker.reset(0);
 
     return true;
 }