Reland "Streamline frame detach"

Source/core/frame/Frame.cpp

Index: Source/core/frame/Frame.cpp
diff --git a/Source/core/frame/Frame.cpp b/Source/core/frame/Frame.cpp
index 302a3e1e1328c2dd483c8800fa04416b5323c0b4..6df0d30636ef370eab9d7cd7f9821c8fe296dce1 100644
--- a/Source/core/frame/Frame.cpp
+++ b/Source/core/frame/Frame.cpp
@@ -70,8 +70,17 @@ Frame::Frame(FrameClient* client, FrameHost* host, FrameOwner* owner)
     frameCounter.increment();
 #endif
 
+    // If the proposed owner has an existing frame, we're in the midst of a swap.
+    // This means the old frame didn't go through the normal detach sequence and therefore
+    // didn't decrement the frame count, so don't increment it here.

[dcheng, 2014/10/13 09:17:16]

Yikes. It would be nice to keep the swap-specific logic more local to
WebFrame::swap(), but I need to think about this more.

+    if (m_owner && m_owner->isLocal() && toHTMLFrameOwnerElement(m_owner)->contentFrame()) {
+        toHTMLFrameOwnerElement(m_owner)->contentFrame()->disconnectOwnerElement();
+        toHTMLFrameOwnerElement(m_owner)->clearContentFrame();
+    } else {
+        m_host->incrementFrameCount();
+    }
+
     if (m_owner) {
-        page()->incrementSubframeCount();
         if (m_owner->isLocal())
             toHTMLFrameOwnerElement(m_owner)->setContentFrame(*this);
     } else {
@@ -102,6 +111,24 @@ void Frame::trace(Visitor* visitor)
     visitor->trace(m_domWindow);
 }
 
+void Frame::detach()
+{
+    // client() should never be null because that means we somehow re-entered
+    // the frame detach code... but it is sometimes.
+    // FIXME: Understand why this is happening so we can document this insanity.
+    // http://crbug.com/371084 is a probable explanation.
+    if (!client())
+        return;
+    // FIXME: Should we enforce the invariant that all pointers nulled in this function
+    // get nulled at the same time?
+    m_host->decrementFrameCount();
+    m_host = nullptr;
+    // After this, we must no longer talk to the client since this clears
+    // its owning reference back to our owning LocalFrame.
+    m_client->detached();
+    m_client = nullptr;
+}
+
 void Frame::detachChildren()
 {
     typedef WillBeHeapVector<RefPtrWillBeMember<Frame> > FrameVector;
@@ -206,8 +233,6 @@ void Frame::disconnectOwnerElement()
     if (m_owner) {

[sof, 2014/10/13 06:51:00]

nit: can fuse the two "if" statements into one.

         if (m_owner->isLocal())
             toHTMLFrameOwnerElement(m_owner)->clearContentFrame();
-        if (page())
-            page()->decrementSubframeCount();
     }
     m_owner = nullptr;
 }