Changeset 140807 in webkit

Timestamp:

Jan 25, 2013 2:48:14 AM (11 years ago)

Author:

commit-queue@webkit.org

Message:

Assert the connectedSubframeCount is consistent and fix over counting
​https://bugs.webkit.org/show_bug.cgi?id=107302

Patch by Elliott Sprehn <Elliott Sprehn> on 2013-01-25
Reviewed by Alexey Proskuryakov.

Source/WebCore:

Add a debug assertion that walks the subtree during frame disconnection
and manually counts the number of connected subframes to assert that the
value from Node::connectedSubframeCount() is the same as if we traversed
through the tree.

In fixing the places where this assertion failed I made document destruction
faster by not walking the entire document looking for frames if the entire
frame tree has been destroyed by way of FrameLoader::detachChildren().
I had inadvertently introduced this improvement in r133933, but then I
regressed it in r140090 when we switched to counting because I didn't
realize we destroy the frame tree separate of frame disconnection on
document unload so all frames could have been destroyed but the counts
left on the ancestors.

I also fixed another overcounting case where the adoption agency algorithm
may call ContainerNode::takeAllChildrenFrom() which in turn calls
ContainerNode::removeAllChildren() and could have left a connected subframe
count on the node even though all the frames had been removed.

This assertion did not uncover any cases of undercounting the number of
frames.

This also fixes a rare edge case where removeChild of an iframe that
was already being unloaded would not unload the frame until the top level
unload was done, and a reparenting of the iframe would not cause it to load.

Test: fast/frames/reparent-in-unload-contentdocument.html

• dom/ContainerNode.cpp:

(WebCore::ContainerNode::removeAllChildren):
(WebCore::ContainerNode::parserInsertBefore):
(WebCore::ContainerNode::parserRemoveChild):
(WebCore::ContainerNode::parserAppendChild):

• dom/ContainerNodeAlgorithms.cpp:

(WebCore):
(WebCore::assertConnectedSubframeCountIsConsistent):

• dom/ContainerNodeAlgorithms.h:

(WebCore):
(WebCore::ChildFrameDisconnector::disconnect):

• dom/Node.cpp:

(WebCore::Node::updateAncestorConnectedSubframeCountForRemoval):
(WebCore):
(WebCore::Node::updateAncestorConnectedSubframeCountForInsertion):

• dom/Node.h:

(Node):

• html/HTMLFrameOwnerElement.cpp:

(WebCore::HTMLFrameOwnerElement::clearContentFrame):
(WebCore):
(WebCore::HTMLFrameOwnerElement::disconnectContentFrame):

• html/HTMLFrameOwnerElement.h:

(HTMLFrameOwnerElement):

LayoutTests:

Add a test that removing an iframe in the middle of unload causes the
contentDocument to become immediately inaccessible.

• fast/frames/reparent-in-unload-contentdocument-expected.txt: Added.
• fast/frames/reparent-in-unload-contentdocument.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/frames/reparent-in-unload-contentdocument-expected.txt (added)
• LayoutTests/fast/frames/reparent-in-unload-contentdocument.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/ContainerNode.cpp (modified) (4 diffs)
• Source/WebCore/dom/ContainerNodeAlgorithms.cpp (modified) (1 diff)
• Source/WebCore/dom/ContainerNodeAlgorithms.h (modified) (2 diffs)
• Source/WebCore/dom/Node.cpp (modified) (1 diff)
• Source/WebCore/dom/Node.h (modified) (1 diff)
• Source/WebCore/html/HTMLFrameOwnerElement.cpp (modified) (2 diffs)
• Source/WebCore/html/HTMLFrameOwnerElement.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2013-01-25  Elliott Sprehn  <esprehn@gmail.com>
+
+        Assert the connectedSubframeCount is consistent and fix over counting
+        https://bugs.webkit.org/show_bug.cgi?id=107302
+
+        Reviewed by Alexey Proskuryakov.
+
+        Add a test that removing an iframe in the middle of unload causes the
+        contentDocument to become immediately inaccessible.
+
+        * fast/frames/reparent-in-unload-contentdocument-expected.txt: Added.
+        * fast/frames/reparent-in-unload-contentdocument.html: Added.
+
 2013-01-25  Kent Tamura  <tkent@chromium.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-01-25  Elliott Sprehn  <esprehn@gmail.com>
+
+        Assert the connectedSubframeCount is consistent and fix over counting
+        https://bugs.webkit.org/show_bug.cgi?id=107302
+
+        Reviewed by Alexey Proskuryakov.
+
+        Add a debug assertion that walks the subtree during frame disconnection
+        and manually counts the number of connected subframes to assert that the
+        value from Node::connectedSubframeCount() is the same as if we traversed
+        through the tree.
+
+        In fixing the places where this assertion failed I made document destruction
+        faster by not walking the entire document looking for frames if the entire
+        frame tree has been destroyed by way of FrameLoader::detachChildren().
+        I had inadvertently introduced this improvement in r133933, but then I
+        regressed it in r140090 when we switched to counting because I didn't
+        realize we destroy the frame tree separate of frame disconnection on
+        document unload so all frames could have been destroyed but the counts
+        left on the ancestors.
+
+        I also fixed another overcounting case where the adoption agency algorithm
+        may call ContainerNode::takeAllChildrenFrom() which in turn calls
+        ContainerNode::removeAllChildren() and could have left a connected subframe
+        count on the node even though all the frames had been removed.
+
+        This assertion did not uncover any cases of undercounting the number of
+        frames.
+
+        This also fixes a rare edge case where removeChild of an iframe that
+        was already being unloaded would not unload the frame until the top level
+        unload was done, and a reparenting of the iframe would not cause it to load.
+
+        Test: fast/frames/reparent-in-unload-contentdocument.html
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeAllChildren):
+        (WebCore::ContainerNode::parserInsertBefore):
+        (WebCore::ContainerNode::parserRemoveChild):
+        (WebCore::ContainerNode::parserAppendChild):
+        * dom/ContainerNodeAlgorithms.cpp:
+        (WebCore):
+        (WebCore::assertConnectedSubframeCountIsConsistent):
+        * dom/ContainerNodeAlgorithms.h:
+        (WebCore):
+        (WebCore::ChildFrameDisconnector::disconnect):
+        * dom/Node.cpp:
+        (WebCore::Node::updateAncestorConnectedSubframeCountForRemoval):
+        (WebCore):
+        (WebCore::Node::updateAncestorConnectedSubframeCountForInsertion):
+        * dom/Node.h:
+        (Node):
+        * html/HTMLFrameOwnerElement.cpp:
+        (WebCore::HTMLFrameOwnerElement::clearContentFrame):
+        (WebCore):
+        (WebCore::HTMLFrameOwnerElement::disconnectContentFrame):
+        * html/HTMLFrameOwnerElement.h:
+        (HTMLFrameOwnerElement):
+
 2013-01-25  Pavel Feldman  <pfeldman@chromium.org>
 

trunk/Source/WebCore/dom/ContainerNode.cpp

@@ -93 +93 @@
 void ContainerNode::removeDetachedChildren()
 {
+    if (connectedSubframeCount()) {
+        for (Node* child = firstChild(); child; child = child->nextSibling())
+            child->updateAncestorConnectedSubframeCountForRemoval();
+    }
     // FIXME: We should be able to ASSERT(!attached()) here: https://bugs.webkit.org/show_bug.cgi?id=107801
     removeDetachedChildrenInContainer<Node, ContainerNode>(this);
@@ -333 +337 @@
     insertBeforeCommon(nextChild, newChild.get());
 
-    if (unsigned count = newChild->connectedSubframeCount()) {
-        for (Node* node = newChild->parentOrHostNode(); node; node = node->parentOrHostNode())
-            node->incrementConnectedSubframeCount(count);
-    }
+    newChild->updateAncestorConnectedSubframeCountForInsertion();
 
     childrenChanged(true, newChild->previousSibling(), nextChild, 1);
@@ -559 +560 @@
     Node* next = oldChild->nextSibling();
 
-    if (unsigned count = oldChild->connectedSubframeCount()) {
-        for (Node* node = oldChild->parentOrHostNode(); node; node = node->parentOrHostNode())
-            node->decrementConnectedSubframeCount(count);
-    }
+    oldChild->updateAncestorConnectedSubframeCountForRemoval();
 
     removeBetween(prev, next, oldChild);
@@ -708 +706 @@
     }
 
-    if (unsigned count = newChild->connectedSubframeCount()) {
-        for (Node* node = newChild->parentOrHostNode(); node; node = node->parentOrHostNode())
-            node->incrementConnectedSubframeCount(count);
-    }
+    newChild->updateAncestorConnectedSubframeCountForInsertion();
 
     childrenChanged(true, last, 0, 1);

trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp

@@ -122 +122 @@
 }
 
+#ifndef NDEBUG
+unsigned assertConnectedSubrameCountIsConsistent(Node* node)
+{
+    unsigned count = 0;
+
+    if (node->isElementNode()) {
+        if (node->isFrameOwnerElement() && toFrameOwnerElement(node)->contentFrame())
+            count++;
+
+        if (ElementShadow* shadow = toElement(node)->shadow()) {
+            for (ShadowRoot* root = shadow->youngestShadowRoot(); root; root = root->olderShadowRoot())
+                count += assertConnectedSubrameCountIsConsistent(root);
+        }
+    }
+
+    for (Node* child = node->firstChild(); child; child = child->nextSibling())
+        count += assertConnectedSubrameCountIsConsistent(child);
+
+    // If we undercount there's possibly a security bug since we'd leave frames
+    // in subtrees outside the document.
+    ASSERT(node->connectedSubframeCount() >= count);
+
+    // If we overcount it's safe, but not optimal because it means we'll traverse
+    // through the document in ChildFrameDisconnector looking for frames that have
+    // already been disconnected.
+    ASSERT(node->connectedSubframeCount() == count);
+
+    return count;
 }
+#endif
+
+}

trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h

@@ -298 +298 @@
 };
 
+#ifndef NDEBUG
+unsigned assertConnectedSubrameCountIsConsistent(Node*);
+#endif
+
 inline void ChildFrameDisconnector::collectFrameOwners(Node* root)
 {
@@ -331 +335 @@
 inline void ChildFrameDisconnector::disconnect(DisconnectPolicy policy)
 {
+#ifndef NDEBUG
+    assertConnectedSubrameCountIsConsistent(m_root);
+#endif
+
     if (!m_root->connectedSubframeCount())
         return;

trunk/Source/WebCore/dom/Node.cpp

@@ -2621 +2621 @@
 }
 
+void Node::updateAncestorConnectedSubframeCountForRemoval() const
+{
+    unsigned count = connectedSubframeCount();
+
+    if (!count)
+        return;
+
+    for (Node* node = parentOrHostNode(); node; node = node->parentOrHostNode())
+        node->decrementConnectedSubframeCount(count);
+}
+
+void Node::updateAncestorConnectedSubframeCountForInsertion() const
+{
+    unsigned count = connectedSubframeCount();
+
+    if (!count)
+        return;
+
+    for (Node* node = parentOrHostNode(); node; node = node->parentOrHostNode())
+        node->incrementConnectedSubframeCount(count);
+}
+
 void Node::registerScopedHTMLStyleChild()
 {

trunk/Source/WebCore/dom/Node.h

@@ -682 +682 @@
     void incrementConnectedSubframeCount(unsigned amount = 1);
     void decrementConnectedSubframeCount(unsigned amount = 1);
+    void updateAncestorConnectedSubframeCountForRemoval() const;
+    void updateAncestorConnectedSubframeCountForInsertion() const;
 
 private:

trunk/Source/WebCore/html/HTMLFrameOwnerElement.cpp

@@ -63 +63 @@
 }
 
+void HTMLFrameOwnerElement::clearContentFrame()
+{
+    if (!m_contentFrame)
+        return;
+
+    m_contentFrame = 0;
+
+    for (ContainerNode* node = this; node; node = node->parentOrHostNode())
+        node->decrementConnectedSubframeCount();
+}
+
 void HTMLFrameOwnerElement::disconnectContentFrame()
 {
@@ -71 +82 @@
     // see if this behavior is really needed as Gecko does not allow this.
     if (Frame* frame = contentFrame()) {
-        for (ContainerNode* node = this; node; node = node->parentOrHostNode())
-            node->decrementConnectedSubframeCount();
-
         RefPtr<Frame> protect(frame);
         frame->loader()->frameDetached();

trunk/Source/WebCore/html/HTMLFrameOwnerElement.h

@@ -44 +44 @@
 
     void setContentFrame(Frame*);
-    void clearContentFrame() { m_contentFrame = 0; }
+    void clearContentFrame();
 
     void disconnectContentFrame();