Avoid inconsistency in Node::inDocument due to DOMSubtreeModified dispatch
authoradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 18 Feb 2012 03:22:10 +0000 (03:22 +0000)
committeradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 18 Feb 2012 03:22:10 +0000 (03:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=76087

Reviewed by Ryosuke Niwa.

Source/WebCore:

Move post-removal notifications after call to Node::removeFromDocument
to avoid inconsistent state of Node::inDocument() and thus avoid
inconsistent state in DocumentOrderedMap.

Tests: fast/dom/getElementById-consistency.html
       fast/dom/getElementById-consistency2.html

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::removeChild):
* svg/SVGTRefElement.cpp:
(WebCore::SVGTRefElement::updateReferencedText): Fixed to work with new timing of DOMSubtreeModified dispatch.

LayoutTests:

* fast/dom/getElementById-consistency-expected.txt: Added.
* fast/dom/getElementById-consistency.html: Added.
* fast/dom/getElementById-consistency2-expected.txt: Added.
* fast/dom/getElementById-consistency2.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/getElementById-consistency-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/getElementById-consistency.html [new file with mode: 0644]
LayoutTests/fast/dom/getElementById-consistency2-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/getElementById-consistency2.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/svg/SVGTRefElement.cpp

index 9b0395b..7b96af6 100644 (file)
@@ -1,3 +1,15 @@
+2012-02-17  Adam Klein  <adamk@chromium.org>
+
+        Avoid inconsistency in Node::inDocument due to DOMSubtreeModified dispatch
+        https://bugs.webkit.org/show_bug.cgi?id=76087
+
+        Reviewed by Ryosuke Niwa.
+
+        * fast/dom/getElementById-consistency-expected.txt: Added.
+        * fast/dom/getElementById-consistency.html: Added.
+        * fast/dom/getElementById-consistency2-expected.txt: Added.
+        * fast/dom/getElementById-consistency2.html: Added.
+
 2012-02-17  Joshua Bell  <jsbell@chromium.org>
 
         IndexedDB: Support overloaded methods that take IDBKey or IDBKeyRange
diff --git a/LayoutTests/fast/dom/getElementById-consistency-expected.txt b/LayoutTests/fast/dom/getElementById-consistency-expected.txt
new file mode 100644 (file)
index 0000000..18568f9
--- /dev/null
@@ -0,0 +1,11 @@
+Test that DOMSubtreeModified listeners cannot cause inDocument to be incorrect
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.contains(document.getElementById('el')) is true
+PASS document.getElementById('el') is null
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/getElementById-consistency.html b/LayoutTests/fast/dom/getElementById-consistency.html
new file mode 100644 (file)
index 0000000..42c17b6
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<script src="../js/resources/js-test-pre.js"></script>
+<div id=container>
+</div>
+<span id=el>
+</span>
+<script>
+description('Test that DOMSubtreeModified listeners cannot cause inDocument to be incorrect');
+
+var counter = 0;
+var container = document.getElementById('container');
+var el = document.getElementById('el');
+function broken() {
+   if (++counter > 2) {
+      document.removeEventListener('DOMSubtreeModified', broken, true);
+      return;
+   }
+   container.appendChild(el);
+}
+
+document.addEventListener("DOMSubtreeModified", broken, true);
+broken();
+shouldBeTrue("document.contains(document.getElementById('el'))");
+el.parentNode.removeChild(el);
+el = null;
+gc();
+shouldBeNull("document.getElementById('el')");
+</script>
+<script src="../js/resources/js-test-post.js"></script>
diff --git a/LayoutTests/fast/dom/getElementById-consistency2-expected.txt b/LayoutTests/fast/dom/getElementById-consistency2-expected.txt
new file mode 100644 (file)
index 0000000..d2e5788
--- /dev/null
@@ -0,0 +1,11 @@
+Test that DOMSubtreeModified listeners cannot cause DocumentOrderedMap to be incorrect
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.contains(el) is true
+PASS document.getElementById('el') is null
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/getElementById-consistency2.html b/LayoutTests/fast/dom/getElementById-consistency2.html
new file mode 100644 (file)
index 0000000..076c3eb
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<script src="../js/resources/js-test-pre.js"></script>
+<div id=container>
+</div>
+<span id=el>
+</span>
+<script>
+description('Test that DOMSubtreeModified listeners cannot cause DocumentOrderedMap to be incorrect');
+
+var counter = 0;
+var container = document.getElementById('container');
+var el = document.getElementById('el');
+function broken() {
+   if (++counter > 2) {
+      document.removeEventListener('DOMSubtreeModified', broken, true);
+      return;
+   }
+   container.appendChild(el);
+}
+
+document.addEventListener("DOMSubtreeModified", broken, true);
+broken();
+shouldBeTrue("document.contains(el)");
+el.parentNode.removeChild(el);
+el = null;
+gc();
+shouldBeNull("document.getElementById('el')");
+</script>
+<script src="../js/resources/js-test-post.js"></script>
index 20aced5..cac3511 100644 (file)
@@ -1,3 +1,22 @@
+2012-02-17  Adam Klein  <adamk@chromium.org>
+
+        Avoid inconsistency in Node::inDocument due to DOMSubtreeModified dispatch
+        https://bugs.webkit.org/show_bug.cgi?id=76087
+
+        Reviewed by Ryosuke Niwa.
+
+        Move post-removal notifications after call to Node::removeFromDocument
+        to avoid inconsistent state of Node::inDocument() and thus avoid
+        inconsistent state in DocumentOrderedMap.
+
+        Tests: fast/dom/getElementById-consistency.html
+               fast/dom/getElementById-consistency2.html
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeChild):
+        * svg/SVGTRefElement.cpp:
+        (WebCore::SVGTRefElement::updateReferencedText): Fixed to work with new timing of DOMSubtreeModified dispatch.
+
 2012-02-17  Joshua Bell  <jsbell@chromium.org>
 
         IndexedDB: Support overloaded methods that take IDBKey or IDBKeyRange
index 84b0a15..0da49bc 100644 (file)
@@ -478,15 +478,15 @@ bool ContainerNode::removeChild(Node* oldChild, ExceptionCode& ec)
     Node* next = child->nextSibling();
     removeBetween(prev, next, child.get());
 
-    // Dispatch post-removal mutation events
-    childrenChanged(false, prev, next, -1);
-    dispatchSubtreeModifiedEvent();
-
     if (child->inDocument())
         child->removedFromDocument();
     else
         child->removedFromTree(true);
 
+    // Dispatch post-removal mutation events
+    childrenChanged(false, prev, next, -1);
+    dispatchSubtreeModifiedEvent();
+
     return child;
 }
 
index b8e4e1d..1570f4a 100644 (file)
@@ -159,10 +159,8 @@ void SVGTRefElement::createShadowSubtree()
 
 void SVGTRefElement::updateReferencedText()
 {
-    Element* target = SVGURIReference::targetElementFromIRIString(href(), document());
-    ASSERT(target);
     String textContent;
-    if (target->parentNode())
+    if (Element* target = SVGURIReference::targetElementFromIRIString(href(), document()))
         textContent = target->textContent();
 
     ASSERT(hasShadowRoot());