Bug 149546

Summary: Node.replaceChild() does not behave according to the specification
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, kangil.han, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar, WebExposed
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
URL: https://dom.spec.whatwg.org/#concept-node-replace
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2015-09-24 16:13:59 PDT
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

This leads to incorrect Mutation Records being queued in some cases.
Comment 1 Chris Dumez 2015-09-24 16:16:46 PDT
<rdar://problem/22571887>
Comment 2 Chris Dumez 2015-09-24 16:16:59 PDT
*** Bug 148781 has been marked as a duplicate of this bug. ***
Comment 3 Chris Dumez 2015-09-24 16:24:39 PDT
Created attachment 261902 [details]
Patch
Comment 4 Ryosuke Niwa 2015-09-24 17:06:37 PDT
Comment on attachment 261902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261902&action=review

r=me provided you add an iframe test as described below.

> Source/WebCore/ChangeLog:15
> +        This patch reverses the order as per the specification. Our new behavior
> +        matches Firefox's behavior.

You should probably mention that this removes the minor optimization.
Also, we should add a test ensuring that onbeforeload event on iframe is called in the expected order.
i.e. if you're replacing an iframe with another iframe, you should get beforeunload on the newChild before on the refChild.

> Source/WebCore/dom/ContainerNode.cpp:430
> +    Ref<Node> removedChild(oldChild);
> +    removeChild(oldChild, ec);

Can we rename the variable to something like childToBeRemoved since removeChild and removedChild only differs by one character "d"?
Comment 5 Ryosuke Niwa 2015-09-24 17:43:21 PDT
I actually take it back. I can't create a reliable test for this :(
Comment 6 Chris Dumez 2015-09-24 19:02:33 PDT
Created attachment 261911 [details]
Patch
Comment 7 WebKit Commit Bot 2015-09-24 19:49:18 PDT
Comment on attachment 261911 [details]
Patch

Clearing flags on attachment: 261911

Committed r190233: <http://trac.webkit.org/changeset/190233>
Comment 8 WebKit Commit Bot 2015-09-24 19:49:25 PDT
All reviewed patches have been landed.  Closing bug.