Streamline frame detach

Source/core/frame/Frame.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Simon Hausmann <hausmann@kde.org>
  *                     2000 Stefan Schimanski <1Stein@gmx.de>
  *                     2001 George Staikos <staikos@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2005 Alexey Proskuryakov <ap@nypop.com>
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
@@ skipping 52 matching lines @@
     , m_owner(owner)
     , m_client(client)
     , m_remotePlatformLayer(0)
 {
     ASSERT(page());
 
 #ifndef NDEBUG
     frameCounter.increment();
 #endif
 
+    m_host->incrementFrameCount();
+
     if (m_owner) {
-        page()->incrementSubframeCount();
         if (m_owner->isLocal())
             toHTMLFrameOwnerElement(m_owner)->setContentFrame(*this);
     } else {
         page()->setMainFrame(this);
     }
 }
 
 Frame::~Frame()
 {
 #if ENABLE(OILPAN)
@@ skipping 10 matching lines @@
 }
 
 void Frame::trace(Visitor* visitor)
 {
     visitor->trace(m_treeNode);
     visitor->trace(m_host);
     visitor->trace(m_owner);
     visitor->trace(m_domWindow);
 }
 
+void Frame::detach()
+{
+    // client() should never be null because that means we somehow re-entered

[dcheng, 2014/09/25 23:35:43]

This was a little hard for me to follow. We needed this comment originally
because this happened after all the other stuff in
FrameLoader::detachFromParent. This still happens 'last' so to speak, but it's
not so obvious. I don't have any good suggestions though...

[Nate Chapin, 2014/09/25 23:57:50]

It may be that this comment should be on the client() null-check in
LocalFrame::detach().

[dcheng, 2014/09/26 00:40:25]

I'm not sure it really makes sense there either. I guess let's just leave it
here =/

+    // 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;
+    m_host->decrementFrameCount();
+    m_host = nullptr;

[dcheng, 2014/09/25 23:35:43]

I don't think we need to do it in this patch, but I wonder if it's worth
enforcing the invariant that host(), client() and page() must all go to null at
the same time?

[Nate Chapin, 2014/09/25 23:57:50]

That seems like a good idea for the future.

[dcheng, 2014/09/26 00:40:25]

Mind adding a FIXME?

[Nate Chapin, 2014/09/26 20:25:51]

Now that I look at it, because page() is directly derived from host(), I think
this invariant is already true.

+    // 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;
     FrameVector childrenToDetach;
     childrenToDetach.reserveCapacity(tree().childCount());
     for (Frame* child = tree().firstChild(); child; child = child->tree().nextSibling())
         childrenToDetach.append(child);
     FrameVector::iterator end = childrenToDetach.end();
     for (FrameVector::iterator it = childrenToDetach.begin(); it != end; ++it)
         (*it)->detach();
@@ skipping 84 matching lines @@
         return true;
 
     return tree().parent()->isRemoteFrame();
 }
 
 void Frame::disconnectOwnerElement()
 {
     if (m_owner) {
         if (m_owner->isLocal())
             toHTMLFrameOwnerElement(m_owner)->clearContentFrame();
-        if (page())
-            page()->decrementSubframeCount();
     }
     m_owner = nullptr;
 }
 
 HTMLFrameOwnerElement* Frame::deprecatedLocalOwner() const
 {
     return m_owner && m_owner->isLocal() ? toHTMLFrameOwnerElement(m_owner) : 0;
 }
 
 } // namespace blink