- Revision
- 190233
- Author
- [email protected]
- Date
- 2015-09-24 19:48:55 -0700 (Thu, 24 Sep 2015)
Log Message
Node.replaceChild() does not behave according to the specification
https://bugs.webkit.org/show_bug.cgi?id=149546
<rdar://problem/22571887>
Reviewed by Ryosuke Niwa.
LayoutTests/imported/w3c:
Rebaseline W3C DOM test now that one more check is passing.
* web-platform-tests/dom/nodes/MutationObserver-childList-expected.txt:
Source/WebCore:
Node.replaceChild() does not behave according to the specification. In
particular, when replacing |child| with |node| we are supposed to remove
|node| from its parent *before* removing |child| from its parent:
- https://dom.spec.whatwg.org/#concept-node-replace (Steps 10 & 11)
This patch reverses the order as per the specification. Our new behavior
matches Firefox's behavior. Note that this patch also remove an
optimization when replacing a child with its next sibling. This
optimization was observable from JS. It seems likely this case is not
common enough for it to be an issue. However, we can revisit if we see
it regresses things.
This leads to incorrect Mutation Records being queued in some cases.
No new tests, already covered by existing test.
* dom/ContainerNode.cpp:
(WebCore::ContainerNode::replaceChild):
LayoutTests:
Update existing test that now throws a different exception.
* fast/events/mutation-during-replace-child-expected.txt:
* fast/events/mutation-during-replace-child.html:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (190232 => 190233)
--- trunk/LayoutTests/ChangeLog 2015-09-25 01:50:04 UTC (rev 190232)
+++ trunk/LayoutTests/ChangeLog 2015-09-25 02:48:55 UTC (rev 190233)
@@ -1,3 +1,16 @@
+2015-09-24 Chris Dumez <[email protected]>
+
+ Node.replaceChild() does not behave according to the specification
+ https://bugs.webkit.org/show_bug.cgi?id=149546
+ <rdar://problem/22571887>
+
+ Reviewed by Ryosuke Niwa.
+
+ Update existing test that now throws a different exception.
+
+ * fast/events/mutation-during-replace-child-expected.txt:
+ * fast/events/mutation-during-replace-child.html:
+
2015-09-24 Beth Dakin <[email protected]>
The same problem happens on El Capitan.
Modified: trunk/LayoutTests/fast/events/mutation-during-replace-child-expected.txt (190232 => 190233)
--- trunk/LayoutTests/fast/events/mutation-during-replace-child-expected.txt 2015-09-25 01:50:04 UTC (rev 190232)
+++ trunk/LayoutTests/fast/events/mutation-during-replace-child-expected.txt 2015-09-25 02:48:55 UTC (rev 190233)
@@ -3,7 +3,7 @@
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-PASS target.replaceChild(newChild, oldChild); threw exception Error: HierarchyRequestError: DOM Exception 3.
+PASS target.replaceChild(newChild, oldChild); threw exception Error: NotFoundError: DOM Exception 8.
PASS successfullyParsed is true
TEST COMPLETE
Modified: trunk/LayoutTests/fast/events/mutation-during-replace-child.html (190232 => 190233)
--- trunk/LayoutTests/fast/events/mutation-during-replace-child.html 2015-09-25 01:50:04 UTC (rev 190232)
+++ trunk/LayoutTests/fast/events/mutation-during-replace-child.html 2015-09-25 02:48:55 UTC (rev 190233)
@@ -24,7 +24,7 @@
newChild.appendChild(target);
}
document.addEventListener("DOMNodeRemoved", handler, false);
-shouldThrow("target.replaceChild(newChild, oldChild);", "'Error: HierarchyRequestError: DOM Exception 3'");
+shouldThrow("target.replaceChild(newChild, oldChild);", "'Error: NotFoundError: DOM Exception 8'");
</script>
<script src=""
</body>
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (190232 => 190233)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2015-09-25 01:50:04 UTC (rev 190232)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2015-09-25 02:48:55 UTC (rev 190233)
@@ -1,5 +1,17 @@
2015-09-24 Chris Dumez <[email protected]>
+ Node.replaceChild() does not behave according to the specification
+ https://bugs.webkit.org/show_bug.cgi?id=149546
+ <rdar://problem/22571887>
+
+ Reviewed by Ryosuke Niwa.
+
+ Rebaseline W3C DOM test now that one more check is passing.
+
+ * web-platform-tests/dom/nodes/MutationObserver-childList-expected.txt:
+
+2015-09-24 Chris Dumez <[email protected]>
+
Rewrite Range::insertNode() as per the latest DOM specification
https://bugs.webkit.org/show_bug.cgi?id=149528
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/dom/nodes/MutationObserver-childList-expected.txt (190232 => 190233)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/dom/nodes/MutationObserver-childList-expected.txt 2015-09-25 01:50:04 UTC (rev 190232)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/dom/nodes/MutationObserver-childList-expected.txt 2015-09-25 02:48:55 UTC (rev 190233)
@@ -22,7 +22,7 @@
PASS childList Node.appendChild: addition outside document tree mutation
PASS childList Node.replaceChild: replacement mutation
PASS childList Node.replaceChild: removal mutation
-FAIL childList Node.replaceChild: internal replacement mutation assert_equals: mutation records must match expected 2 but got 1
+PASS childList Node.replaceChild: internal replacement mutation
PASS childList Node.removeChild: removal mutation
PASS Range (r70) is created
PASS childList Range.deleteContents: child removal mutation
Modified: trunk/Source/WebCore/ChangeLog (190232 => 190233)
--- trunk/Source/WebCore/ChangeLog 2015-09-25 01:50:04 UTC (rev 190232)
+++ trunk/Source/WebCore/ChangeLog 2015-09-25 02:48:55 UTC (rev 190233)
@@ -1,5 +1,32 @@
2015-09-24 Chris Dumez <[email protected]>
+ Node.replaceChild() does not behave according to the specification
+ https://bugs.webkit.org/show_bug.cgi?id=149546
+ <rdar://problem/22571887>
+
+ Reviewed by Ryosuke Niwa.
+
+ Node.replaceChild() does not behave according to the specification. In
+ particular, when replacing |child| with |node| we are supposed to remove
+ |node| from its parent *before* removing |child| from its parent:
+ - https://dom.spec.whatwg.org/#concept-node-replace (Steps 10 & 11)
+
+ This patch reverses the order as per the specification. Our new behavior
+ matches Firefox's behavior. Note that this patch also remove an
+ optimization when replacing a child with its next sibling. This
+ optimization was observable from JS. It seems likely this case is not
+ common enough for it to be an issue. However, we can revisit if we see
+ it regresses things.
+
+ This leads to incorrect Mutation Records being queued in some cases.
+
+ No new tests, already covered by existing test.
+
+ * dom/ContainerNode.cpp:
+ (WebCore::ContainerNode::replaceChild):
+
+2015-09-24 Chris Dumez <[email protected]>
+
Unreviewed, roll out r187615 as it seems to have caused a ~1% PLT regression.
<rdar://problem/22657123>
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (190232 => 190233)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2015-09-25 01:50:04 UTC (rev 190232)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2015-09-25 02:48:55 UTC (rev 190233)
@@ -414,50 +414,48 @@
ChildListMutationScope mutation(*this);
- RefPtr<Node> next = oldChild.nextSibling();
+ RefPtr<Node> refChild = oldChild.nextSibling();
+ if (refChild.get() == newChild.ptr())
+ refChild = refChild->nextSibling();
- // Remove the node we're replacing
- Ref<Node> removedChild(oldChild);
- removeChild(oldChild, ec);
+ NodeVector targets;
+ collectChildrenAndRemoveFromOldParent(newChild, targets, ec);
if (ec)
return false;
- if (next && (next->previousSibling() == newChild.ptr() || next == newChild.ptr())) // nothing to do
- return true;
-
- // Does this one more time because removeChild() fires a MutationEvent.
+ // Does this one more time because collectChildrenAndRemoveFromOldParent() fires a MutationEvent.
if (!checkPreReplacementValidity(*this, newChild, oldChild, ec))
return false;
- NodeVector targets;
- collectChildrenAndRemoveFromOldParent(newChild, targets, ec);
+ // Remove the node we're replacing.
+ Ref<Node> protectOldChild(oldChild);
+ removeChild(oldChild, ec);
if (ec)
return false;
- // Does this yet another check because collectChildrenAndRemoveFromOldParent() fires a MutationEvent.
+ // Does this one more time because removeChild() fires a MutationEvent.
if (!checkPreReplacementValidity(*this, newChild, oldChild, ec))
return false;
InspectorInstrumentation::willInsertDOMNode(document(), *this);
- // Add the new child(ren)
+ // Add the new child(ren).
for (auto& child : targets) {
// Due to arbitrary code running in response to a DOM mutation event it's
- // possible that "next" is no longer a child of "this".
+ // possible that "refChild" is no longer a child of "this".
// It's also possible that "child" has been inserted elsewhere.
// In either of those cases, we'll just stop.
- if (next && next->parentNode() != this)
+ if (refChild && refChild->parentNode() != this)
break;
if (child->parentNode())
break;
treeScope().adoptIfNeeded(child.ptr());
- // Add child before "next".
{
NoEventDispatchAssertion assertNoEventDispatch;
- if (next)
- insertBeforeCommon(*next, child.get());
+ if (refChild)
+ insertBeforeCommon(*refChild, child.get());
else
appendChildCommon(child);
}
