Bug 149546 - Node.replaceChild() does not behave according to the specification
Summary: Node.replaceChild() does not behave according to the specification
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://dom.spec.whatwg.org/#concept-...
Keywords: InRadar, WebExposed
: 148781 (view as bug list)
Depends on:
Reported: 2015-09-24 16:13 PDT by Chris Dumez
Modified: 2015-09-24 19:49 PDT (History)
6 users (show)

See Also:

Patch (8.27 KB, patch)
2015-09-24 16:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.69 KB, patch)
2015-09-24 19:02 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:

This leads to incorrect Mutation Records being queued in some cases.
Comment 1 Chris Dumez 2015-09-24 16:16:46 PDT
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]
Comment 4 Ryosuke Niwa 2015-09-24 17:06:37 PDT
Comment on attachment 261902 [details]

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]
Comment 7 WebKit Commit Bot 2015-09-24 19:49:18 PDT
Comment on attachment 261911 [details]

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.