Last Comment Bug 647603 - a.replaceChild(b, b) does not remove and re-insert the affected node
: a.replaceChild(b, b) does not remove and re-insert the affected node
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla7
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on: 725784 726440
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-03 14:04 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-02-12 09:10 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (8.02 KB, patch)
2011-06-29 01:49 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Review
Patch v2 (12.24 KB, patch)
2011-06-29 15:23 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bzbarsky: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2011-04-03 14:04:42 PDT
The current DOM Core spec says that if you call replaceChild() with both arguments the same, the argument should be removed from its parent and then re-added at the original position:

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-node-replacechild

This does not actually match browsers other than IE, as demonstrated by the following test case:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/904

The test case checks whether the node was actually removed by a) registering a DOMNodeRemoved handler and checking if it fires, and b) creating a Range that surrounds the node and checking whether it's collapsed afterwards.

Chrome 11 dev logs nothing.  Firefox 4.0 and Opera 11 log

log: removed for insertBefore
log: range collapsed after insertBefore
log: removed for appendChild
log: range collapsed after appendChild

IE 9 logs

log: removed for insertBefore
log: range collapsed after insertBefore
log: removed for replaceChild
log: removed for replaceChild
log: range collapsed after replaceChild
log: removed for appendChild
log: range collapsed after appendChild

The upshot is that when asked to do a no-op insertBefore() or appendChild(), all engines except WebKit remove the node from the DOM and re-add it, as required by the spec.  But for a no-op replaceChild(), Gecko (along with WebKit and Opera) seems to just leave the node alone.

Since all engines but IE agree on how to behave, I considered filing a bug against the spec instead.  But browser behavior seems pointlessly inconsistent -- the spec's/IE's behavior makes more sense, with all three methods behaving the same.  So I'm filing a bug on Gecko instead to change to match IE.  If you think the spec should change instead, WONTFIX this and I'll file a spec bug.

(Needless to say, this is not likely a bug that is of much importance to users.  But I'm writing DOM Range mutation tests now, and I need to know whether we're going to change the spec or not so I know what result to require in the tests.)
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-04 15:54:43 PDT
So our current behavior is actually consistent for insertBefore and replaceChild.  Both are no-ops if the first and second arguments are equal.

Note that in our implementation doing the other thing will take some serious surgery on the node insertion code to use the next/previous links instead of indices.  Then again, we may want to do that anyway....
Comment 2 :Aryeh Gregor (away until August 15) 2011-04-05 10:42:28 PDT
(In reply to comment #1)
> So our current behavior is actually consistent for insertBefore and
> replaceChild.  Both are no-ops if the first and second arguments are equal.

Hmm, I didn't think to test that case.  Updated:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/915

Gecko, WebKit, and Opera all agree on this case too, same as with replaceChild().  IE9 seems to fire a DOM mutation event but not collapse the range (??).  So I think everyone should either match IE's behavior and the current spec, or WebKit's behavior.  The current spec is slightly simpler, and it rarely matters, so I'd say we should leave the spec as-is, yes?
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-05 10:45:15 PDT
I don't really have a strong opinion here.  As a I said, the current spec is not that trivial to implement, but might not be that bad either.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2011-04-05 22:06:03 PDT
I sort of agree that the spec definition is the most consistent and thus preferable.

Boris, why is this hard to fix? Shouldn't just removing this test and possibly adjust how we adjust indexes do it?

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#4057
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-05 23:15:55 PDT
The hard part is the index-adjustment, because the reference node we're using right now will suddenly be removed from the parent.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-29 01:49:52 PDT
Created attachment 542744 [details] [diff] [review]
Patch to fix

This should do it. We no longer need the reference node to keep track of our current insert position now that mutation events aren't mucking around with our state any more.

In fact, I noticed that we're keeping track of the refContent local way more than we need to. In fact, we only need the reference node to calculate the insert position and to pass to IsAllowedAsChild. So if we change IsAllowedAsChild to take a nsINode instead, then refContent can go away completely.

So technically, the only change here needed to fix the bug is to remove the |if (aRefChild == aNewChild)| early-return (and to remove the assertion that checks that we took that shortcut). The rest is cleanup.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-29 15:23:44 PDT
Created attachment 542975 [details] [diff] [review]
Patch v2

This includes tests and an additional fix. Turns out that for something like

a.replaceChild(b, b)

we fire DOMNodeRemoved twice for b, which I kind'a suspected. This isn't new (and in fact was worst before this patch since we fired DOMNodeInserted 0 times), but it caused the tests I wrote to trip up.

So the patch now fixes this too.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-29 23:07:00 PDT
Comment on attachment 542975 [details] [diff] [review]
Patch v2

r=me
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-29 23:17:17 PDT
Checked in http://hg.mozilla.org/integration/mozilla-inbound/rev/c2f48684b9f5
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-29 23:17:45 PDT
Oh, and thanks for the speedy review!
Comment 11 Marco Bonardo [::mak] 2011-06-30 06:15:14 PDT
http://hg.mozilla.org/mozilla-central/rev/c2f48684b9f5
Comment 12 Ioana (away) 2011-09-06 08:44:22 PDT
Verified fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

When loading the test case from comment 2 you get:
removed for insertBefore(self, null)
range collapsed after insertBefore(self, null)
removed for replaceChild
range collapsed after replaceChild
removed for appendChild
range collapsed after appendChild
removed for insertBefore(self, self)
range collapsed after insertBefore(self, self).

Note You need to log in before you can comment on or make changes to this bug.