NoEventDispatchAssertion in ContainerNode::removeChildren is too strict
authormorrita@google.com <morrita@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 17 Jan 2013 05:25:14 +0000 (05:25 +0000)
committermorrita@google.com <morrita@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 17 Jan 2013 05:25:14 +0000 (05:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=106985

Reviewed by Ryosuke Niwa.

Source/WebCore:

This change narrowed the lifetime of NoEventDispatchAssertion in removeChildren().
It is as safe as other mutation method even after this change: childrenChanged() and
ChildNodeRemovalNotifier are used outside the assertion scope.

Test: svg/custom/use-mutation-crash.xhtml

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::removeChildren):

LayoutTests:

* svg/custom/use-mutation-crash-expected.txt: Added.
* svg/custom/use-mutation-crash.xhtml: Added.

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

LayoutTests/ChangeLog
LayoutTests/svg/custom/use-mutation-crash-expected.txt [new file with mode: 0644]
LayoutTests/svg/custom/use-mutation-crash.xhtml [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp

index 9ff4c1cdbae53ca9682a09cc34e4aab802f7683e..8c92e4b1c7e653aef4a3529c94aa1928c50bb0c1 100644 (file)
@@ -1,3 +1,13 @@
+2013-01-16  Hajime Morrita  <morrita@google.com>
+
+        NoEventDispatchAssertion in ContainerNode::removeChildren is too strict
+        https://bugs.webkit.org/show_bug.cgi?id=106985
+
+        Reviewed by Ryosuke Niwa.
+
+        * svg/custom/use-mutation-crash-expected.txt: Added.
+        * svg/custom/use-mutation-crash.xhtml: Added.
+
 2013-01-16  MORITA Hajime  <morrita@google.com>
 
         Attr.ownerDocument should change if its parent's owner did
diff --git a/LayoutTests/svg/custom/use-mutation-crash-expected.txt b/LayoutTests/svg/custom/use-mutation-crash-expected.txt
new file mode 100644 (file)
index 0000000..ed63619
--- /dev/null
@@ -0,0 +1 @@
+PASS. 
diff --git a/LayoutTests/svg/custom/use-mutation-crash.xhtml b/LayoutTests/svg/custom/use-mutation-crash.xhtml
new file mode 100644 (file)
index 0000000..da16729
--- /dev/null
@@ -0,0 +1,19 @@
+<html>
+<body>
+<pre id="console">PASS.</pre>
+<svg id="tCFSVG" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns="http://www.w3.org/2000/svg">
+
+<g id="target"> </g>
+<use xlink:href="#target"></use> 
+
+<script><![CDATA[
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+target = document.getElementById("target");
+target.textContent = "Hello";
+]]></script>
+
+</svg>
+</body>
+</html>
index 464967f30ea2295c0957e18d22b8a313d8452eda..c0d22559012ce90ac23782d17b1610bfc3ccf203 100644 (file)
@@ -1,3 +1,19 @@
+2013-01-16  Hajime Morrita  <morrita@google.com>
+
+        NoEventDispatchAssertion in ContainerNode::removeChildren is too strict
+        https://bugs.webkit.org/show_bug.cgi?id=106985
+
+        Reviewed by Ryosuke Niwa.
+
+        This change narrowed the lifetime of NoEventDispatchAssertion in removeChildren().
+        It is as safe as other mutation method even after this change: childrenChanged() and
+        ChildNodeRemovalNotifier are used outside the assertion scope.
+
+        Test: svg/custom/use-mutation-crash.xhtml
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeChildren):
+
 2013-01-16  Shinya Kawanaka  <shinyak@chromium.org>
 
         [Refactoring] HTMLTextFormControlElement should use shadowHost instead of shadowAncestorNode
index 9ac14801c07124d8350d6a84434df48cb3516c8e..01759e2abf5f8f9e5c875b4ade9cf25492157c6a 100644 (file)
@@ -582,43 +582,42 @@ void ContainerNode::removeChildren()
     Vector<RefPtr<Node>, 10> removedChildren;
     {
         WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
-        NoEventDispatchAssertion assertNoEventDispatch;
-        removedChildren.reserveInitialCapacity(childNodeCount());
-        while (RefPtr<Node> n = m_firstChild) {
-            Node* next = n->nextSibling();
-
-            // Remove the node from the tree before calling detach or removedFromDocument (4427024, 4129744).
-            // removeChild() does this after calling detach(). There is no explanation for
-            // this discrepancy between removeChild() and its optimized version removeChildren().
-            n->setPreviousSibling(0);
-            n->setNextSibling(0);
-            n->setParentOrHostNode(0);
-            document()->adoptIfNeeded(n.get());
-
-            m_firstChild = next;
-            if (n == m_lastChild)
-                m_lastChild = 0;
-            removedChildren.append(n.release());
-        }
+        {
+            NoEventDispatchAssertion assertNoEventDispatch;
+            removedChildren.reserveInitialCapacity(childNodeCount());
+            while (RefPtr<Node> n = m_firstChild) {
+                Node* next = n->nextSibling();
+
+                // Remove the node from the tree before calling detach or removedFromDocument (4427024, 4129744).
+                // removeChild() does this after calling detach(). There is no explanation for
+                // this discrepancy between removeChild() and its optimized version removeChildren().
+                n->setPreviousSibling(0);
+                n->setNextSibling(0);
+                n->setParentOrHostNode(0);
+                document()->adoptIfNeeded(n.get());
+
+                m_firstChild = next;
+                if (n == m_lastChild)
+                    m_lastChild = 0;
+                removedChildren.append(n.release());
+            }
 
-        size_t removedChildrenCount = removedChildren.size();
-        size_t i;
-
-        // Detach the nodes only after properly removed from the tree because
-        // a. detaching requires a proper DOM tree (for counters and quotes for
-        // example) and during the previous loop the next sibling still points to
-        // the node being removed while the node being removed does not point back
-        // and does not point to the same parent as its next sibling.
-        // b. destroying Renderers of standalone nodes is sometimes faster.
-        for (i = 0; i < removedChildrenCount; ++i) {
-            Node* removedChild = removedChildren[i].get();
-            if (removedChild->attached())
-                removedChild->detach();
+            // Detach the nodes only after properly removed from the tree because
+            // a. detaching requires a proper DOM tree (for counters and quotes for
+            // example) and during the previous loop the next sibling still points to
+            // the node being removed while the node being removed does not point back
+            // and does not point to the same parent as its next sibling.
+            // b. destroying Renderers of standalone nodes is sometimes faster.
+            for (size_t i = 0; i < removedChildren.size(); ++i) {
+                Node* removedChild = removedChildren[i].get();
+                if (removedChild->attached())
+                    removedChild->detach();
+            }
         }
 
-        childrenChanged(false, 0, 0, -static_cast<int>(removedChildrenCount));
-
-        for (i = 0; i < removedChildrenCount; ++i)
+        childrenChanged(false, 0, 0, -static_cast<int>(removedChildren.size()));
+        
+        for (size_t i = 0; i < removedChildren.size(); ++i)
             ChildNodeRemovalNotifier(this).notify(removedChildren[i].get());
     }