Node::replaceChild() can create bad DOM topology with MutationEvent

https://bugs.webkit.org/show_bug.cgi?id=92619 Reviewed by Ryosuke Niwa. Source/WebCore: Node::replaceChild() calls insertBeforeCommon() after dispatching a MutationEvent event for removeChild(). But insertBeforeCommon() expects call sites to check the invariant and doesn't have suffient check. So a MutationEvent handler can let some bad tree topology to slip into insertBeforeCommon(). This change adds a guard for checking the invariant using checkReplaceChild() between removeChild() and insertBeforeCommon(). Test: fast/events/mutation-during-replace-child.html * dom/ContainerNode.cpp: (WebCore::ContainerNode::replaceChild): Added a guard. LayoutTests: * fast/events/mutation-during-replace-child-expected.txt: Added. * fast/events/mutation-during-replace-child.html: Added. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@124156 268f45cc-cd09-0410-ab3c-d52691b4dbfc
mikewest · Jul 31, 2012 · bb1ab57 · bb1ab57
1 parent 258144a
commit bb1ab57
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 0 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2012-07-30  MORITA Hajime  <morrita@google.com>
+
+        Node::replaceChild() can create bad DOM topology with MutationEvent
+        https://bugs.webkit.org/show_bug.cgi?id=92619
+
+        Reviewed by Ryosuke Niwa.
+
+        * fast/events/mutation-during-replace-child-expected.txt: Added.
+        * fast/events/mutation-during-replace-child.html: Added.
+
 2012-07-30  Kent Tamura  <tkent@chromium.org>
 
         Fix a popup position issue of fast/forms/date/calendar-picker-appearance.html

diff --git a/LayoutTests/fast/events/mutation-during-replace-child-expected.txt b/LayoutTests/fast/events/mutation-during-replace-child-expected.txt
@@ -0,0 +1,10 @@
+Ensures that replaceChild() throws an exception if mutation even handler does something wrong
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS target.replaceChild(newChild, oldChild); threw exception Error: HIERARCHY_REQUEST_ERR: DOM Exception 3.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/events/mutation-during-replace-child.html b/LayoutTests/fast/events/mutation-during-replace-child.html
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<div>
+  <div id="target">
+    <b></b><b id="oldChild"></b><b></b>
+  </div>
+  <div id="newChild"></div>
+</div>
+
+<script>
+description("Ensures that replaceChild() throws an exception if mutation even handler does something wrong");
+var target = document.getElementById('target');
+var oldChild = document.getElementById('oldChild');
+var newChild = document.getElementById('newChild');
+
+function handler(){
+    document.removeEventListener("DOMNodeRemoved", handler, false);
+    newChild.parentNode.removeChild(newChild);
+    target.parentNode.removeChild(target);
+    newChild.appendChild(target);
+}   
+document.addEventListener("DOMNodeRemoved", handler, false);
+shouldThrow("target.replaceChild(newChild, oldChild);",  "'Error: HIERARCHY_REQUEST_ERR: DOM Exception 3'");
+</script>
+<script src="../js/resources/js-test-post.js"></script>
+</body>
+</html>
+
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,24 @@
+2012-07-30  MORITA Hajime  <morrita@google.com>
+
+        Node::replaceChild() can create bad DOM topology with MutationEvent
+        https://bugs.webkit.org/show_bug.cgi?id=92619
+
+        Reviewed by Ryosuke Niwa.
+
+        Node::replaceChild() calls insertBeforeCommon() after dispatching
+        a MutationEvent event for removeChild(). But insertBeforeCommon()
+        expects call sites to check the invariant and doesn't have
+        suffient check. So a MutationEvent handler can let some bad tree
+        topology to slip into insertBeforeCommon().
+
+        This change adds a guard for checking the invariant using
+        checkReplaceChild() between removeChild() and insertBeforeCommon().
+
+        Test: fast/events/mutation-during-replace-child.html
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::replaceChild): Added a guard.
+
 2012-07-30  Ryosuke Niwa  <rniwa@webkit.org>
 
         Qt Windows build fix attempt after r124098.

diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp
@@ -271,6 +271,11 @@ bool ContainerNode::replaceChild(PassRefPtr<Node> newChild, Node* oldChild, Exce
     if (next && (next->previousSibling() == newChild || next == newChild)) // nothing to do
         return true;
 
+    // Does this one more time because removeChild() fires a MutationEvent.
+    checkReplaceChild(newChild.get(), oldChild, ec);
+    if (ec)
+        return false;
+
     NodeVector targets;
     collectChildrenAndRemoveFromOldParent(newChild.get(), targets, ec);
     if (ec)