2010-08-20 Tony Gentilcore <tonyg@chromium.org>
authortonyg@chromium.org <tonyg@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 21 Aug 2010 00:25:43 +0000 (00:25 +0000)
committertonyg@chromium.org <tonyg@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 21 Aug 2010 00:25:43 +0000 (00:25 +0000)
        Reviewed by Adam Barth.

        Crash in WebCore::Node::createRendererIfNeeded()
        https://bugs.webkit.org/show_bug.cgi?id=44129

        * html5lib/resources/adoption02.dat: A crazy DOM from https://bugs.webkig.org/show_bug.cgi?id=44170 which previously crashed. The expectation doesn't appear to be correct, but it behaves the same in Firefox 4, so I'm going to file a spec bug.
        * fast/parser/remove-misnested-iframe-in-beforeload-expected.txt:
        * fast/parser/remove-misnested-iframe-in-beforeload.html: Misnest a node in a table and remove it during its beforeload handler. The node should not be in the tree.
        * fast/parser/remove-misnested-iframe-parent-in-beforeload-expected.txt:
        * fast/parser/remove-misnested-iframe-parent-in-beforeload.html: Misnest a node in a table and remove its parent during its beforeload handler. The adoption agency should have already changed its parent to the one before the table and it and its parent should be removed.
2010-08-20  Tony Gentilcore  <tonyg@chromium.org>

        Reviewed by Adam Barth.

        Crash in WebCore::Node::createRendererIfNeeded()
        https://bugs.webkit.org/show_bug.cgi?id=44129

        * dom/ContainerNode.cpp:
        (WebCore::ContainerNode::insertBefore): Factor out core bit to insertBetween.
        (WebCore::ContainerNode::insertBetween): Factored out of insertBefore.
        (WebCore::ContainerNode::parserInsertBefore): Similar to insertBefore, but doesn't handle reparenting or dispatch DOM mutation events.
        (WebCore::ContainerNode::removeChild): Factor out core bit to removeBetween.
        (WebCore::ContainerNode::removeBetween): Facotred out of removeChild.
        (WebCore::ContainerNode::parserRemoveChild): Similar to removeChild, but doesn't handle reparenting or dispatch DOM mutation events.
        (WebCore::ContainerNode::addChildCommon):
        (WebCore::ContainerNode::parserAddChild):
        (WebCore::ContainerNode::legacyParserAddChild):
        * dom/ContainerNode.h:
        * dom/Node.cpp:
        (WebCore::Node::parserRemoveChild):
        (WebCore::Node::parserInsertBefore):
        * dom/Node.h:
        * html/HTMLConstructionSite.cpp:
        (WebCore::HTMLConstructionSite::attach):
        (WebCore::HTMLConstructionSite::attachAtSite): Use new parserInsertBefore.
        * html/HTMLTreeBuilder.cpp:
        (WebCore::HTMLTreeBuilder::passTokenToLegacyParser):
        (WebCore::HTMLTreeBuilder::reparentChildren):
        (WebCore::HTMLTreeBuilder::callTheAdoptionAgency): Was missing a key bit from the spec about removing the old parent if it exists.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/parser/remove-misnested-iframe-in-beforeload-expected.txt [new file with mode: 0755]
LayoutTests/fast/parser/remove-misnested-iframe-in-beforeload.html [new file with mode: 0644]
LayoutTests/fast/parser/remove-misnested-iframe-parent-in-beforeload-expected.txt [new file with mode: 0755]
LayoutTests/fast/parser/remove-misnested-iframe-parent-in-beforeload.html [new file with mode: 0644]
LayoutTests/html5lib/resources/adoption01.dat
LayoutTests/html5lib/runner-expected.txt
WebCore/ChangeLog
WebCore/dom/ContainerNode.cpp
WebCore/dom/ContainerNode.h
WebCore/dom/Node.cpp
WebCore/dom/Node.h
WebCore/html/HTMLConstructionSite.cpp
WebCore/html/HTMLTreeBuilder.cpp

index edc4a50..7227f00 100644 (file)
@@ -1,3 +1,16 @@
+2010-08-20  Tony Gentilcore  <tonyg@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        Crash in WebCore::Node::createRendererIfNeeded()
+        https://bugs.webkit.org/show_bug.cgi?id=44129
+
+        * html5lib/resources/adoption02.dat: A crazy DOM from https://bugs.webkig.org/show_bug.cgi?id=44170 which previously crashed. The expectation doesn't appear to be correct, but it behaves the same in Firefox 4, so I'm going to file a spec bug.
+        * fast/parser/remove-misnested-iframe-in-beforeload-expected.txt:
+        * fast/parser/remove-misnested-iframe-in-beforeload.html: Misnest a node in a table and remove it during its beforeload handler. The node should not be in the tree.
+        * fast/parser/remove-misnested-iframe-parent-in-beforeload-expected.txt:
+        * fast/parser/remove-misnested-iframe-parent-in-beforeload.html: Misnest a node in a table and remove its parent during its beforeload handler. The adoption agency should have already changed its parent to the one before the table and it and its parent should be removed.
+
 2010-08-20  Martin Robinson  <mrobinson@igalia.com>
 
         Reviewed by Joseph Pecoraro.
diff --git a/LayoutTests/fast/parser/remove-misnested-iframe-in-beforeload-expected.txt b/LayoutTests/fast/parser/remove-misnested-iframe-in-beforeload-expected.txt
new file mode 100755 (executable)
index 0000000..7a94cb3
--- /dev/null
@@ -0,0 +1,13 @@
+| <html>
+|   <head>
+|     <script>
+|       src="../../resources/dump-as-markup.js"
+|     "
+"
+|   <body>
+|     <table>
+|       "
+
+"
+|     "
+"
diff --git a/LayoutTests/fast/parser/remove-misnested-iframe-in-beforeload.html b/LayoutTests/fast/parser/remove-misnested-iframe-in-beforeload.html
new file mode 100644 (file)
index 0000000..b7a6429
--- /dev/null
@@ -0,0 +1,4 @@
+<script src="../../resources/dump-as-markup.js"></script>
+<table>
+<iframe onbeforeload="event.target.parentNode.removeChild(event.target)"></iframe>
+</table>
diff --git a/LayoutTests/fast/parser/remove-misnested-iframe-parent-in-beforeload-expected.txt b/LayoutTests/fast/parser/remove-misnested-iframe-parent-in-beforeload-expected.txt
new file mode 100755 (executable)
index 0000000..70dd618
--- /dev/null
@@ -0,0 +1,9 @@
+| <html>
+|   <head>
+|     <script>
+|       src="../../resources/dump-as-markup.js"
+|     "
+"
+|   <body>
+|     "
+"
diff --git a/LayoutTests/fast/parser/remove-misnested-iframe-parent-in-beforeload.html b/LayoutTests/fast/parser/remove-misnested-iframe-parent-in-beforeload.html
new file mode 100644 (file)
index 0000000..e5d2c8d
--- /dev/null
@@ -0,0 +1,6 @@
+<script src="../../resources/dump-as-markup.js"></script>
+<div>
+<table>
+<iframe onbeforeload="event.target.parentNode.parentNode.removeChild(event.target.parentNode)"></iframe>
+</table>
+</div>
index 0362f37..0c886d5 100644 (file)
 |         <tr>
 |           <td>
 |             "B"
+
+#data
+<a><svg><tr><input></a>
+#errors
+#document
+| <html>
+|   <head>
+|   <body>
+|     <a>
+|       <svg svg>
+|     <svg tr>
+|       <a>
+|   <svg input>
+|     <a>
index c9ae245..cc0912f 100644 (file)
@@ -202,7 +202,7 @@ resources/comments01.dat: PASS
 resources/adoption01.dat:
 12
 
-Test 12 of 12 in resources/adoption01.dat failed. Input:
+Test 12 of 13 in resources/adoption01.dat failed. Input:
 <table>A<td>B</td>C</table>
 Got:
 | <html>
index f0da790..75cb008 100644 (file)
@@ -1,3 +1,33 @@
+2010-08-20  Tony Gentilcore  <tonyg@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        Crash in WebCore::Node::createRendererIfNeeded()
+        https://bugs.webkit.org/show_bug.cgi?id=44129
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::insertBefore): Factor out core bit to insertBetween.
+        (WebCore::ContainerNode::insertBetween): Factored out of insertBefore.
+        (WebCore::ContainerNode::parserInsertBefore): Similar to insertBefore, but doesn't handle reparenting or dispatch DOM mutation events.
+        (WebCore::ContainerNode::removeChild): Factor out core bit to removeBetween.
+        (WebCore::ContainerNode::removeBetween): Facotred out of removeChild.
+        (WebCore::ContainerNode::parserRemoveChild): Similar to removeChild, but doesn't handle reparenting or dispatch DOM mutation events.
+        (WebCore::ContainerNode::addChildCommon):
+        (WebCore::ContainerNode::parserAddChild):
+        (WebCore::ContainerNode::legacyParserAddChild):
+        * dom/ContainerNode.h:
+        * dom/Node.cpp:
+        (WebCore::Node::parserRemoveChild):
+        (WebCore::Node::parserInsertBefore):
+        * dom/Node.h:
+        * html/HTMLConstructionSite.cpp:
+        (WebCore::HTMLConstructionSite::attach):
+        (WebCore::HTMLConstructionSite::attachAtSite): Use new parserInsertBefore.
+        * html/HTMLTreeBuilder.cpp:
+        (WebCore::HTMLTreeBuilder::passTokenToLegacyParser):
+        (WebCore::HTMLTreeBuilder::reparentChildren):
+        (WebCore::HTMLTreeBuilder::callTheAdoptionAgency): Was missing a key bit from the spec about removing the old parent if it exists.
+
 2010-08-20  Kinuko Yasuda  <kinuko@chromium.org>
 
         Unreviewed; build fix.  Had included wrong version of build file.
index e559ba7..e919494 100644 (file)
@@ -134,31 +134,12 @@ bool ContainerNode::insertBefore(PassRefPtr<Node> newChild, Node* refChild, Exce
         if (child->parentNode())
             break;
 
-        ASSERT(!child->nextSibling());
-        ASSERT(!child->previousSibling());
-
-        // Add child before "next".
-        forbidEventDispatch();
-        Node* prev = next->previousSibling();
-        ASSERT(m_lastChild != prev);
-        next->setPreviousSibling(child);
-        if (prev) {
-            ASSERT(m_firstChild != next);
-            ASSERT(prev->nextSibling() == next);
-            prev->setNextSibling(child);
-        } else {
-            ASSERT(m_firstChild == next);
-            m_firstChild = child;
-        }
-        child->setParent(this);
-        child->setPreviousSibling(prev);
-        child->setNextSibling(next.get());
-        allowEventDispatch();
+        insertBeforeCommon(next.get(), child);
 
         // Send notification about the children change.
         childrenChanged(false, refChildPreviousSibling.get(), next.get(), 1);
         notifyChildInserted(child);
-                
+
         // Add child to the rendering tree.
         if (attached() && !child->attached() && child->parent() == this) {
             if (shouldLazyAttach)
@@ -176,6 +157,57 @@ bool ContainerNode::insertBefore(PassRefPtr<Node> newChild, Node* refChild, Exce
     return true;
 }
 
+void ContainerNode::insertBeforeCommon(Node* nextChild, Node* newChild)
+{
+    ASSERT(newChild);
+    ASSERT(!newChild->parent()); // Use insertBefore if you need to handle reparenting (and want DOM mutation events).
+    ASSERT(!newChild->nextSibling());
+    ASSERT(!newChild->previousSibling());
+
+    forbidEventDispatch();
+    Node* prev = nextChild->previousSibling();
+    ASSERT(m_lastChild != prev);
+    nextChild->setPreviousSibling(newChild);
+    if (prev) {
+        ASSERT(m_firstChild != nextChild);
+        ASSERT(prev->nextSibling() == nextChild);
+        prev->setNextSibling(newChild);
+    } else {
+        ASSERT(m_firstChild == nextChild);
+        m_firstChild = newChild;
+    }
+    newChild->setParent(this);
+    newChild->setPreviousSibling(prev);
+    newChild->setNextSibling(nextChild);
+    allowEventDispatch();
+}
+
+void ContainerNode::parserInsertBefore(PassRefPtr<Node> newChild, Node* nextChild)
+{
+    ASSERT(newChild);
+    ASSERT(nextChild);
+    ASSERT(nextChild->parentNode() == this);
+
+    NodeVector targets;
+    collectTargetNodes(newChild.get(), targets);
+    if (targets.isEmpty())
+        return;
+
+    if (nextChild->previousSibling() == newChild || nextChild == newChild) // nothing to do
+        return;
+
+    RefPtr<Node> next = nextChild;
+    RefPtr<Node> nextChildPreviousSibling = nextChild->previousSibling();
+    for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
+        Node* child = it->get();
+
+        insertBeforeCommon(next.get(), child);
+
+        childrenChanged(true, nextChildPreviousSibling.get(), nextChild, 1);
+        notifyChildInserted(child);
+    }
+}
+
 bool ContainerNode::replaceChild(PassRefPtr<Node> newChild, Node* oldChild, ExceptionCode& ec, bool shouldLazyAttach)
 {
     // Check that this node is not "floating".
@@ -367,31 +399,9 @@ bool ContainerNode::removeChild(Node* oldChild, ExceptionCode& ec)
     // that no callers call with ref count == 0 and parent = 0 (as of this
     // writing, there are definitely callers who call that way).
 
-    forbidEventDispatch();
-
-    // Remove from rendering tree
-    if (child->attached())
-        child->detach();
-
-    // Remove the child
-    Node *prev, *next;
-    prev = child->previousSibling();
-    next = child->nextSibling();
-
-    if (next)
-        next->setPreviousSibling(prev);
-    if (prev)
-        prev->setNextSibling(next);
-    if (m_firstChild == child)
-        m_firstChild = next;
-    if (m_lastChild == child)
-        m_lastChild = prev;
-
-    child->setPreviousSibling(0);
-    child->setNextSibling(0);
-    child->setParent(0);
-
-    allowEventDispatch();
+    Node* prev = child->previousSibling();
+    Node* next = child->nextSibling();
+    removeBetween(prev, next, child.get());
 
     // Dispatch post-removal mutation events
     childrenChanged(false, prev, next, -1);
@@ -405,6 +415,50 @@ bool ContainerNode::removeChild(Node* oldChild, ExceptionCode& ec)
     return child;
 }
 
+void ContainerNode::removeBetween(Node* previousChild, Node* nextChild, Node* oldChild)
+{
+    ASSERT(oldChild);
+    ASSERT(oldChild->parentNode() == this);
+
+    forbidEventDispatch();
+
+    // Remove from rendering tree
+    if (oldChild->attached())
+        oldChild->detach();
+
+    if (nextChild)
+        nextChild->setPreviousSibling(previousChild);
+    if (previousChild)
+        previousChild->setNextSibling(nextChild);
+    if (m_firstChild == oldChild)
+        m_firstChild = nextChild;
+    if (m_lastChild == oldChild)
+        m_lastChild = previousChild;
+
+    oldChild->setPreviousSibling(0);
+    oldChild->setNextSibling(0);
+    oldChild->setParent(0);
+
+    allowEventDispatch();
+}
+
+void ContainerNode::parserRemoveChild(Node* oldChild)
+{
+    ASSERT(oldChild);
+    ASSERT(oldChild->parentNode() == this);
+
+    Node* prev = oldChild->previousSibling();
+    Node* next = oldChild->nextSibling();
+
+    removeBetween(prev, next, oldChild);
+
+    childrenChanged(true, prev, next, -1);
+    if (oldChild->inDocument())
+        oldChild->removedFromDocument();
+    else
+        oldChild->removedFromTree(true);
+}
+
 // this differs from other remove functions because it forcibly removes all the children,
 // regardless of read-only status or event exceptions, e.g.
 bool ContainerNode::removeChildren()
@@ -536,13 +590,16 @@ bool ContainerNode::appendChild(PassRefPtr<Node> newChild, ExceptionCode& ec, bo
 
 void ContainerNode::addChildCommon(Node* newChild)
 {
-    ASSERT(!newChild->parent()); // Use appendChild if you need to handle reparenting.
+    ASSERT(newChild);
+    ASSERT(!newChild->parent()); // Use appendChild if you need to handle reparenting (and want DOM mutation events).
+
     forbidEventDispatch();
     Node* last = m_lastChild;
     // FIXME: This method should take a PassRefPtr.
     appendChildToContainer<Node, ContainerNode>(newChild, this);
     allowEventDispatch();
 
+    // FIXME: Why doesn't this use notifyChildInserted(newChild) instead?
     document()->incDOMTreeVersion();
     if (inDocument())
         newChild->insertedIntoDocument();
@@ -552,17 +609,12 @@ void ContainerNode::addChildCommon(Node* newChild)
 void ContainerNode::parserAddChild(PassRefPtr<Node> newChild)
 {
     ASSERT(newChild);
-    // This function is only used during parsing.
-    // It does not send any DOM mutation events or handle reparenting.
-
     addChildCommon(newChild.get());
 }
 
 ContainerNode* ContainerNode::legacyParserAddChild(PassRefPtr<Node> newChild)
 {
     ASSERT(newChild);
-    // This function is only used during parsing.
-    // It does not send any DOM mutation events.
 
     // Check for consistency with DTD, but only when parsing HTML.
     if (document()->isHTMLDocument() && !childAllowed(newChild.get()))
index a27d95b..1114d3b 100644 (file)
@@ -48,8 +48,13 @@ public:
     virtual bool removeChild(Node* child, ExceptionCode&);
     virtual bool appendChild(PassRefPtr<Node> newChild, ExceptionCode&, bool shouldLazyAttach = false);
 
+    // These methods are only used during parsing.
+    // They don't send DOM mutation events or handle reparenting.
+    // However, arbitrary code may be run by beforeload handlers.
     virtual ContainerNode* legacyParserAddChild(PassRefPtr<Node>);
     virtual void parserAddChild(PassRefPtr<Node>);
+    virtual void parserRemoveChild(Node*);
+    virtual void parserInsertBefore(PassRefPtr<Node> newChild, Node* refChild);
 
     bool hasChildNodes() const { return m_firstChild; }
     virtual void attach();
@@ -97,6 +102,8 @@ protected:
 private:
     // FIXME: This should take a PassRefPtr.
     void addChildCommon(Node*);
+    void removeBetween(Node* previousChild, Node* nextChild, Node* oldChild);
+    void insertBeforeCommon(Node* nextChild, Node* oldChild);
 
     static void dispatchPostAttachCallbacks();
 
index 0bc8d9e..89d1392 100644 (file)
@@ -651,6 +651,16 @@ void Node::parserAddChild(PassRefPtr<Node>)
     ASSERT_NOT_REACHED();
 }
 
+void Node::parserRemoveChild(PassRefPtr<Node>)
+{
+    ASSERT_NOT_REACHED();
+}
+
+void Node::parserInsertBefore(PassRefPtr<Node>, Node*)
+{
+    ASSERT_NOT_REACHED();
+}
+
 bool Node::isContentEditable() const
 {
     return parent() && parent()->isContentEditable();
index ec16f05..3f09162 100644 (file)
@@ -267,6 +267,8 @@ public:
     // addChild is tied into the logic of the LegacyHTMLTreeBuilder.  We need
     // a "clean" version to use for the HTML5 version of the HTMLTreeBuilder.
     virtual void parserAddChild(PassRefPtr<Node>);
+    virtual void parserRemoveChild(PassRefPtr<Node>);
+    virtual void parserInsertBefore(PassRefPtr<Node> newChild, Node* nextChild);
 
     // Called by the parser when this element's close tag is reached,
     // signaling that all child tags have been parsed and added.
index 9e43b70..4c0207c 100644 (file)
@@ -92,7 +92,7 @@ PassRefPtr<ChildType> HTMLConstructionSite::attach(Node* parent, PassRefPtr<Chil
     // doesn't.  It feels like we're missing a concept somehow.
     if (shouldFosterParent()) {
         fosterParent(child.get());
-        ASSERT(child->attached() || !child->parent()->attached());
+        ASSERT(child->attached() || !child->parent() || !child->parent()->attached());
         return child.release();
     }
 
@@ -118,10 +118,7 @@ void HTMLConstructionSite::attachAtSite(const AttachmentSite& site, PassRefPtr<N
     RefPtr<Node> child = prpChild;
 
     if (site.nextChild) {
-        // FIXME: We need an insertElement which does not send mutation events.
-        ExceptionCode ec = 0;
-        site.parent->insertBefore(child, site.nextChild, ec);
-        ASSERT(!ec);
+        site.parent->parserInsertBefore(child, site.nextChild);
         if (site.parent->attached() && !child->attached())
             child->attach();
         return;
index 78055f7..f36b6ab 100644 (file)
@@ -1756,9 +1756,10 @@ void HTMLTreeBuilder::reparentChildren(Element* oldParent, Element* newParent)
     Node* child = oldParent->firstChild();
     while (child) {
         Node* nextChild = child->nextSibling();
-        ExceptionCode ec;
-        newParent->appendChild(child, ec);
-        ASSERT(!ec);
+        oldParent->parserRemoveChild(child);
+        newParent->parserAddChild(child);
+        if (newParent->attached() && !child->attached())
+            child->attach();
         child = nextChild;
     }
 }
@@ -1829,15 +1830,18 @@ void HTMLTreeBuilder::callTheAdoptionAgency(AtomicHTMLToken& token)
             if (lastNode == furthestBlock)
                 bookmark.moveToAfter(nodeEntry);
             // 6.6
-            // Use appendChild instead of parserAddChild to handle possible reparenting.
-            ExceptionCode ec;
-            node->element()->appendChild(lastNode->element(), ec, true);
-            ASSERT(!ec);
+            if (Element* parent = lastNode->element()->parentElement())
+                parent->parserRemoveChild(lastNode->element());
+            node->element()->parserAddChild(lastNode->element());
+            if (lastNode->element()->parentElement()->attached() && !lastNode->element()->attached())
+                lastNode->element()->lazyAttach();
             // 6.7
             lastNode = node;
         }
         // 7
         const AtomicString& commonAncestorTag = commonAncestor->localName();
+        if (Element* parent = lastNode->element()->parentElement())
+            parent->parserRemoveChild(lastNode->element());
         // FIXME: If this moves to HTMLConstructionSite, this check should use
         // causesFosterParenting(tagName) instead.
         if (commonAncestorTag == tableTag
@@ -1845,9 +1849,9 @@ void HTMLTreeBuilder::callTheAdoptionAgency(AtomicHTMLToken& token)
             || isTableBodyContextTag(commonAncestorTag))
             m_tree.fosterParent(lastNode->element());
         else {
-            ExceptionCode ec;
-            commonAncestor->appendChild(lastNode->element(), ec, true);
-            ASSERT(!ec);
+            commonAncestor->parserAddChild(lastNode->element());
+            if (lastNode->element()->parentElement()->attached() && !lastNode->element()->attached())
+                lastNode->element()->lazyAttach();
         }
         // 8
         RefPtr<Element> newElement = m_tree.createHTMLElementFromElementRecord(formattingElementRecord);