Remove bogus assertions from ChildListMutationScope
authoradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Sep 2012 01:27:52 +0000 (01:27 +0000)
committeradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Sep 2012 01:27:52 +0000 (01:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=97372

Reviewed by Ryosuke Niwa.

Source/WebCore:

Some asserts (and their accompanying comment) were trying to enforce
proper usage of ChildListMutationScope from WebCore, but in the
presence of MutationEvents they could fail due to arbitrary script
execution.

This change gets rid of those asserts and adds tests exercising
the (pre-existing) codepaths for handling these out-of-order cases.
Without this patch, these tests ASSERT in debug builds.

Tests: fast/mutation/added-out-of-order.html
       fast/mutation/removed-out-of-order.html

* dom/ChildListMutationScope.cpp:
(WebCore::ChildListMutationAccumulator::childAdded):
(WebCore::ChildListMutationAccumulator::willRemoveChild):
* dom/ChildListMutationScope.h:
(WebCore):

LayoutTests:

* fast/mutation/added-out-of-order-expected.txt: Added.
* fast/mutation/added-out-of-order.html: Added.
* fast/mutation/removed-out-of-order-expected.txt: Added.
* fast/mutation/removed-out-of-order.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/mutation/added-out-of-order-expected.txt [new file with mode: 0644]
LayoutTests/fast/mutation/added-out-of-order.html [new file with mode: 0644]
LayoutTests/fast/mutation/removed-out-of-order-expected.txt [new file with mode: 0644]
LayoutTests/fast/mutation/removed-out-of-order.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ChildListMutationScope.cpp
Source/WebCore/dom/ChildListMutationScope.h

index 94eec66..0a2c122 100644 (file)
@@ -1,3 +1,15 @@
+2012-09-21  Adam Klein  <adamk@chromium.org>
+
+        Remove bogus assertions from ChildListMutationScope
+        https://bugs.webkit.org/show_bug.cgi?id=97372
+
+        Reviewed by Ryosuke Niwa.
+
+        * fast/mutation/added-out-of-order-expected.txt: Added.
+        * fast/mutation/added-out-of-order.html: Added.
+        * fast/mutation/removed-out-of-order-expected.txt: Added.
+        * fast/mutation/removed-out-of-order.html: Added.
+
 2012-09-21  Dan Bernstein  <mitz@apple.com>
 
         REGRESSION (r129176): Incorrect line breaking when kerning occurs between a space and the following character
diff --git a/LayoutTests/fast/mutation/added-out-of-order-expected.txt b/LayoutTests/fast/mutation/added-out-of-order-expected.txt
new file mode 100644 (file)
index 0000000..dffbbb3
--- /dev/null
@@ -0,0 +1,19 @@
+Test MutationEvents interfering with MutationObservers: adding nodes 'out of order'
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS mutations.length is 3
+PASS mutations[0].addedNodes.length is 0
+PASS mutations[0].removedNodes.length is 1
+PASS mutations[0].removedNodes[0].tagName is 'SPAN'
+PASS mutations[1].addedNodes.length is 1
+PASS mutations[1].removedNodes.length is 0
+PASS mutations[1].addedNodes[0].tagName is 'DIV'
+PASS mutations[2].addedNodes.length is 1
+PASS mutations[2].removedNodes.length is 0
+PASS mutations[2].addedNodes[0].nodeValue is 'hello world'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/mutation/added-out-of-order.html b/LayoutTests/fast/mutation/added-out-of-order.html
new file mode 100644 (file)
index 0000000..043e9a4
--- /dev/null
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<div id="sandbox" style="display:none"><span></span></div>
+<script src="../js/resources/js-test-pre.js"></script>
+<script>
+description("Test MutationEvents interfering with MutationObservers: adding nodes 'out of order'");
+var sandbox = document.getElementById('sandbox');
+var inserted = false;
+sandbox.addEventListener('DOMNodeRemoved', function() {
+    if (!inserted) {
+        sandbox.appendChild(document.createElement('div'));
+        inserted = true;
+    }
+});
+var observer = new WebKitMutationObserver(function(){});
+observer.observe(sandbox, {childList: true});
+sandbox.textContent = 'hello world';
+
+var mutations = observer.takeRecords();
+shouldBe("mutations.length", "3");
+shouldBe("mutations[0].addedNodes.length", "0");
+shouldBe("mutations[0].removedNodes.length", "1");
+shouldBe("mutations[0].removedNodes[0].tagName", "'SPAN'");
+shouldBe("mutations[1].addedNodes.length", "1");
+shouldBe("mutations[1].removedNodes.length", "0");
+shouldBe("mutations[1].addedNodes[0].tagName", "'DIV'");
+shouldBe("mutations[2].addedNodes.length", "1");
+shouldBe("mutations[2].removedNodes.length", "0");
+shouldBe("mutations[2].addedNodes[0].nodeValue", "'hello world'");
+</script>
+<script src="../js/resources/js-test-post.js"></script>
diff --git a/LayoutTests/fast/mutation/removed-out-of-order-expected.txt b/LayoutTests/fast/mutation/removed-out-of-order-expected.txt
new file mode 100644 (file)
index 0000000..55dfc01
--- /dev/null
@@ -0,0 +1,17 @@
+Test MutationEvents interfering with MutationObservers: removing nodes 'out of order'
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS mutations.length is 2
+PASS mutations[0].addedNodes.length is 1
+PASS mutations[0].removedNodes.length is 0
+PASS mutations[0].addedNodes[0].tagName is 'B'
+PASS mutations[1].addedNodes.length is 1
+PASS mutations[1].removedNodes.length is 1
+PASS mutations[1].removedNodes[0].tagName is 'B'
+PASS mutations[1].addedNodes[0].tagName is 'I'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/mutation/removed-out-of-order.html b/LayoutTests/fast/mutation/removed-out-of-order.html
new file mode 100644 (file)
index 0000000..423de78
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<div id="sandbox" style="display:none"></div>
+<script src="../js/resources/js-test-pre.js"></script>
+<script>
+description("Test MutationEvents interfering with MutationObservers: removing nodes 'out of order'");
+var sandbox = document.getElementById('sandbox');
+var removed = false;
+sandbox.addEventListener('DOMNodeInserted', function() {
+    if (!removed) {
+        sandbox.removeChild(sandbox.firstChild);
+        removed = true;
+    }
+});
+var observer = new WebKitMutationObserver(function(){});
+observer.observe(sandbox, {childList: true});
+sandbox.innerHTML = '<b></b><i></i>';
+
+var mutations = observer.takeRecords();
+shouldBe("mutations.length", "2");
+shouldBe("mutations[0].addedNodes.length", "1");
+shouldBe("mutations[0].removedNodes.length", "0");
+shouldBe("mutations[0].addedNodes[0].tagName", "'B'");
+shouldBe("mutations[1].addedNodes.length", "1");
+shouldBe("mutations[1].removedNodes.length", "1");
+shouldBe("mutations[1].removedNodes[0].tagName", "'B'");
+shouldBe("mutations[1].addedNodes[0].tagName", "'I'");
+</script>
+</script>
+<script src="../js/resources/js-test-post.js"></script>
index b13a294..fa3b419 100644 (file)
@@ -1,3 +1,28 @@
+2012-09-21  Adam Klein  <adamk@chromium.org>
+
+        Remove bogus assertions from ChildListMutationScope
+        https://bugs.webkit.org/show_bug.cgi?id=97372
+
+        Reviewed by Ryosuke Niwa.
+
+        Some asserts (and their accompanying comment) were trying to enforce
+        proper usage of ChildListMutationScope from WebCore, but in the
+        presence of MutationEvents they could fail due to arbitrary script
+        execution.
+
+        This change gets rid of those asserts and adds tests exercising
+        the (pre-existing) codepaths for handling these out-of-order cases.
+        Without this patch, these tests ASSERT in debug builds.
+
+        Tests: fast/mutation/added-out-of-order.html
+               fast/mutation/removed-out-of-order.html
+
+        * dom/ChildListMutationScope.cpp:
+        (WebCore::ChildListMutationAccumulator::childAdded):
+        (WebCore::ChildListMutationAccumulator::willRemoveChild):
+        * dom/ChildListMutationScope.h:
+        (WebCore):
+
 2012-09-21  Dan Bernstein  <mitz@apple.com>
 
         REGRESSION (r129176): Incorrect line breaking when kerning occurs between a space and the following character
index bdff209..44762bd 100644 (file)
@@ -87,10 +87,10 @@ inline bool ChildListMutationAccumulator::isAddedNodeInOrder(Node* child)
 
 void ChildListMutationAccumulator::childAdded(PassRefPtr<Node> prpChild)
 {
+    ASSERT(hasObservers());
+
     RefPtr<Node> child = prpChild;
 
-    ASSERT(hasObservers());
-    ASSERT(isAddedNodeInOrder(child.get()));
     if (!isAddedNodeInOrder(child.get()))
         enqueueMutationRecord();
 
@@ -110,10 +110,10 @@ inline bool ChildListMutationAccumulator::isRemovedNodeInOrder(Node* child)
 
 void ChildListMutationAccumulator::willRemoveChild(PassRefPtr<Node> prpChild)
 {
+    ASSERT(hasObservers());
+
     RefPtr<Node> child = prpChild;
 
-    ASSERT(hasObservers());
-    ASSERT(m_addedNodes.isEmpty() && isRemovedNodeInOrder(child.get()));
     if (!m_addedNodes.isEmpty() || !isRemovedNodeInOrder(child.get()))
         enqueueMutationRecord();
 
index 005519a..a47ee55 100644 (file)
@@ -46,11 +46,6 @@ namespace WebCore {
 class MutationObserverInterestGroup;
 
 // ChildListMutationAccumulator is not meant to be used directly; ChildListMutationScope is the public interface.
-//
-// ChildListMutationAccumulator expects that all removals from a parent take place in order
-// and precede any additions. If this is violated (i.e. because of code changes elsewhere
-// in WebCore) it will likely result in both (a) ASSERTions failing, and (b) mutation records
-// being enqueued for delivery before the outer-most scope closes.
 class ChildListMutationAccumulator : public RefCounted<ChildListMutationAccumulator> {
 public:
     static PassRefPtr<ChildListMutationAccumulator> getOrCreate(Node*);