Assert the connectedSubframeCount is consistent and fix over counting
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Jan 2013 10:48:14 +0000 (10:48 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Jan 2013 10:48:14 +0000 (10:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=107302

Patch by Elliott Sprehn <esprehn@gmail.com> 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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@140807 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/frames/reparent-in-unload-contentdocument-expected.txt [new file with mode: 0644]
LayoutTests/fast/frames/reparent-in-unload-contentdocument.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/dom/ContainerNodeAlgorithms.cpp
Source/WebCore/dom/ContainerNodeAlgorithms.h
Source/WebCore/dom/Node.cpp
Source/WebCore/dom/Node.h
Source/WebCore/html/HTMLFrameOwnerElement.cpp
Source/WebCore/html/HTMLFrameOwnerElement.h

index 044c1a0..2cca58a 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         INPUT_MULTIPLE_FIELDS_UI: Inconsistent value of aria-valuetext attribute
diff --git a/LayoutTests/fast/frames/reparent-in-unload-contentdocument-expected.txt b/LayoutTests/fast/frames/reparent-in-unload-contentdocument-expected.txt
new file mode 100644 (file)
index 0000000..1034926
--- /dev/null
@@ -0,0 +1,11 @@
+PASS frame.contentDocument is null
+PASS frame.contentDocument is not null
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
+--------
+Frame: '<!--framePath //<!--frame0-->-->'
+--------
+Should be visible.
diff --git a/LayoutTests/fast/frames/reparent-in-unload-contentdocument.html b/LayoutTests/fast/frames/reparent-in-unload-contentdocument.html
new file mode 100644 (file)
index 0000000..f44fc48
--- /dev/null
@@ -0,0 +1,44 @@
+<!DOCTYPE html>
+
+<script src="../js/resources/js-test-pre.js"></script>
+
+<div id="a">
+    <div id="b">
+    </div>
+</div>
+
+<div id="c">
+</div>
+
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.dumpChildFramesAsText();
+    testRunner.waitUntilDone();
+}
+
+var frame = document.createElement('iframe');
+var a = document.getElementById('a');
+var b = document.getElementById('b');
+var c = document.getElementById('c');
+
+onload = function() {
+    b.appendChild(frame);
+
+    frame.onload = function() {
+        frame.contentDocument.body.innerHTML = 'Should be visible.';
+    };
+
+    frame.contentWindow.onunload = function() {
+        b.parentNode.removeChild(b);
+        shouldBeNull('frame.contentDocument');
+        c.appendChild(b);
+        shouldNotBe('frame.contentDocument', 'null');
+        isSuccessfullyParsed();
+        if (window.testRunner)
+            testRunner.notifyDone();
+    };
+
+    a.parentNode.removeChild(a);
+};
+</script>
index dadb969..809bb0d 100644 (file)
@@ -1,3 +1,62 @@
+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>
 
         Web Inspector: inspector slows down pages with many anonymous scripts.
index c123f49..873bf66 100644 (file)
@@ -92,6 +92,10 @@ static void collectChildrenAndRemoveFromOldParent(Node* node, NodeVector& nodes,
 
 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);
 }
@@ -332,10 +336,7 @@ void ContainerNode::parserInsertBefore(PassRefPtr<Node> newChild, Node* nextChil
 
     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);
     ChildNodeInsertionNotifier(this).notify(newChild.get());
@@ -558,10 +559,7 @@ void ContainerNode::parserRemoveChild(Node* oldChild)
     Node* prev = oldChild->previousSibling();
     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);
 
@@ -707,10 +705,7 @@ void ContainerNode::parserAppendChild(PassRefPtr<Node> newChild)
         treeScope()->adoptIfNeeded(newChild.get());
     }
 
-    if (unsigned count = newChild->connectedSubframeCount()) {
-        for (Node* node = newChild->parentOrHostNode(); node; node = node->parentOrHostNode())
-            node->incrementConnectedSubframeCount(count);
-    }
+    newChild->updateAncestorConnectedSubframeCountForInsertion();
 
     childrenChanged(true, last, 0, 1);
     ChildNodeInsertionNotifier(this).notify(newChild.get());
index 080b0f3..09d7b22 100644 (file)
@@ -121,4 +121,35 @@ void ChildFrameDisconnector::Target::disconnect()
     toFrameOwnerElement(m_owner.get())->disconnectContentFrame();
 }
 
+#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
+
 }
index 9ef1056..c1bc32c 100644 (file)
@@ -297,6 +297,10 @@ private:
     Node* m_root;
 };
 
+#ifndef NDEBUG
+unsigned assertConnectedSubrameCountIsConsistent(Node*);
+#endif
+
 inline void ChildFrameDisconnector::collectFrameOwners(Node* root)
 {
     if (!root->connectedSubframeCount())
@@ -330,6 +334,10 @@ inline void ChildFrameDisconnector::disconnectCollectedFrameOwners()
 
 inline void ChildFrameDisconnector::disconnect(DisconnectPolicy policy)
 {
+#ifndef NDEBUG
+    assertConnectedSubrameCountIsConsistent(m_root);
+#endif
+
     if (!m_root->connectedSubframeCount())
         return;
 
index 200f19b..4116901 100644 (file)
@@ -2620,6 +2620,28 @@ void Node::decrementConnectedSubframeCount(unsigned amount)
     rareData()->decrementConnectedSubframeCount(amount);
 }
 
+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()
 {
     setHasScopedHTMLStyleChild(true);
index 161e5ec..92fb2b9 100644 (file)
@@ -681,6 +681,8 @@ public:
     unsigned connectedSubframeCount() const;
     void incrementConnectedSubframeCount(unsigned amount = 1);
     void decrementConnectedSubframeCount(unsigned amount = 1);
+    void updateAncestorConnectedSubframeCountForRemoval() const;
+    void updateAncestorConnectedSubframeCountForInsertion() const;
 
 private:
     enum NodeFlags {
index fb698ea..2118fb6 100644 (file)
@@ -62,6 +62,17 @@ void HTMLFrameOwnerElement::setContentFrame(Frame* frame)
         node->incrementConnectedSubframeCount();
 }
 
+void HTMLFrameOwnerElement::clearContentFrame()
+{
+    if (!m_contentFrame)
+        return;
+
+    m_contentFrame = 0;
+
+    for (ContainerNode* node = this; node; node = node->parentOrHostNode())
+        node->decrementConnectedSubframeCount();
+}
+
 void HTMLFrameOwnerElement::disconnectContentFrame()
 {
     ASSERT(hasCustomCallbacks());
@@ -70,9 +81,6 @@ void HTMLFrameOwnerElement::disconnectContentFrame()
     // reach up into this document and then attempt to look back down. We should
     // 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();
         frame->disconnectOwnerElement();
index 15383ae..d07ece0 100644 (file)
@@ -43,7 +43,7 @@ public:
     Document* contentDocument() const;
 
     void setContentFrame(Frame*);
-    void clearContentFrame() { m_contentFrame = 0; }
+    void clearContentFrame();
 
     void disconnectContentFrame();