Closed Bug 798974 Opened 8 years ago Closed 8 years ago

MutationObserver childList contains superflous nodes

Categories

(Core :: DOM: Core & HTML, defect)

15 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
firefox16 --- affected
firefox17 --- verified
firefox18 --- verified
firefox19 --- verified
firefox-esr10 --- unaffected

People

(Reporter: sunil, Assigned: smaug)

Details

(Keywords: testcase)

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.4 (KHTML, like Gecko) Chrome/22.0.1229.79 Safari/537.4

Steps to reproduce:

I registered for Mutation Observer (registering for all character data/attribute/childList changes on the entire doc's subtree and it's iframes) on a document for the following site :
http://www.cnn.com/2012/10/04/politics/debate-main/index.html?hpt=hp_t2

After the complete page is loaded, at regular intervals the JS on the page adds a script tag and after some time removes the script tag.

(Sorry don't have a more contained use case).




Actual results:

When the script tag gets added, my Mutation Observer callback gets invoked with 1 Mutation Record. It correctly includes the information about the node that got added.

When the script tag gets removed, my Mutation Observer callback gets invoked with 2 Mutation Records. It correctly includes the information about the script tag that got removed, but the 2nd Mutation Record includes info about all the existing nodes (in the addedNodes). 



Expected results:

The 2nd Mutation Record shouldn't have been present, as those nodes never got added.
Severity: normal → major
OS: Linux → Windows XP
Severity: major → normal
Component: Untriaged → DOM
Product: Firefox → Core
Minimal testcase please.
The test case is simple, adds a script node and after some time removes it by setting it's outerHTML to '' (empty string).

In Firefox, first get a MutationRecord with info about the node that got added. After some time get a MutationRecord with the info about the node that got removed, but also get a MutationRecord with all the siblings of the script node in the addedNodes (this is the bug).

Same code in Chrome works correctly.
Attachment #670267 - Attachment mime type: text/plain → text/html
Assignee: nobody → bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Thanks for the report. This is an issue when inserting empty document fragments.
Patch coming.
Attached patch patchSplinter Review
Attachment #670326 - Flags: review?(jonas)
Comment on attachment 670326 [details] [diff] [review]
patch

Thanks a lot for a quick fix. Which Firefox version will this make to? Is Firefox 16 possible?
(In reply to Sunil from comment #7)
> Thanks a lot for a quick fix. Which Firefox version will this make to? Is
> Firefox 16 possible?

Sorry, the earliest this could be fixed is in Firefox 17, but perhaps later.
https://hg.mozilla.org/mozilla-central/rev/c200ffe94912
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 670326 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug in the original MutationObserver implementation
User impact if declined:
  wrongly behaving API
Testing completed (on m-c, etc.):
  landed m-c, tests added
Risk to taking this patch (and alternatives if risky):
  The patch affects *only* MutationObserver behavior,
  which is the reason I even ask to take the patch to branches
String or UUID changes made by this patch:
  NA
Attachment #670326 - Flags: approval-mozilla-beta?
Attachment #670326 - Flags: approval-mozilla-aurora?
Comment on attachment 670326 [details] [diff] [review]
patch

Approving for Aurora since it's so early in the cycle. Not clear why we'd take this on Beta though - doesn't appear to have significant user impact. A lack of user impact isn't necessarily a good reason to take a fix either ;)
Attachment #670326 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Alex Keybl [:akeybl] from comment #11)
> Not clear why we'd
> take this on Beta though - doesn't appear to have significant user impact. A
> lack of user impact isn't necessarily a good reason to take a fix either ;)

Can anyone speak to the user impact if this goes unfixed in 17?
Not sure about user impact but could have an impact on extension developers. 

Mutation Events are highly discouraged (such extensions might not even be accepted by Mozilla for public distribution), and if there are outstanding bugs like this in the replacement Mutation Observer, makes life difficult for extension developers.

I vote for getting this fixed in 17.
"outstanding" is a bit strong. This is effectively only about using outerHTML to remove a node from its parent (which is guaranteed to be slower than node.parentNode.removeChild(node) ).

But the patch should be safe and we want to get rid of use of mutation events asap, so
I'd take this to FF 17.
Comment on attachment 670326 [details] [diff] [review]
patch

Thank you for the clarification, please go ahead and uplift to beta.
Attachment #670326 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Oops, wrong a= in the commit message. certainly shouldn't be kkeybl, and should be lsblakk
Keywords: testcase, verifyme
FF 16.0.2:
[12:28:54.090] Dealing with mutation 0 
[12:28:54.091] Removed nodes::0, Added nodes::1 
[12:28:54.091] Added nodes:: 
[12:28:54.091] Node[0]<script></script> 
[12:28:55.087] Dealing with mutation 0 
[12:28:55.087] Removed nodes::1, Added nodes::3 
[12:28:55.088] Added nodes:: 
[12:28:55.088] Node[0]undefined 
[12:28:55.089] Node[1]<h1>This is the Heading1 within body</h1> 
[12:28:55.089] Node[2]undefined

FF 17b5:
[12:30:24.622] Dealing with mutation 0
[12:30:24.622] Removed nodes::0, Added nodes::1
[12:30:24.622] Added nodes::
[12:30:24.623] Node[0]<script></script>
[12:30:25.590] Dealing with mutation 0
[12:30:25.590] Removed nodes::1, Added nodes::0
[12:30:25.590] Added nodes::

Verified fixed on FF 17.
(In reply to Mihaela Velimiroviciu [QA] (:mihaela) from comment #19)
> FF 16.0.2:
> [12:28:54.090] Dealing with mutation 0 
> [12:28:54.091] Removed nodes::0, Added nodes::1 
> [12:28:54.091] Added nodes:: 
> [12:28:54.091] Node[0]<script></script> 
> [12:28:55.087] Dealing with mutation 0 
> [12:28:55.087] Removed nodes::1, Added nodes::3 
> [12:28:55.088] Added nodes:: 
> [12:28:55.088] Node[0]undefined 
> [12:28:55.089] Node[1]<h1>This is the Heading1 within body</h1> 
> [12:28:55.089] Node[2]undefined
> 
> FF 17b5:
> [12:30:24.622] Dealing with mutation 0
> [12:30:24.622] Removed nodes::0, Added nodes::1
> [12:30:24.622] Added nodes::
> [12:30:24.623] Node[0]<script></script>
> [12:30:25.590] Dealing with mutation 0
> [12:30:25.590] Removed nodes::1, Added nodes::0
> [12:30:25.590] Added nodes::
> 
> Verified fixed on FF 17.

I verified this on FF 18 beta 2 on x_86 Windows XP:
Mozilla/5.0 (Windows NT 5.1; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)

[16:50:41.575] Dealing with mutation 0
[16:50:41.576] Removed nodes::0, Added nodes::1
[16:50:41.576] Added nodes::
[16:50:41.576] Node[0]<script></script>
[16:50:42.576] Dealing with mutation 0
[16:50:42.576] Removed nodes::1, Added nodes::0
[16:50:42.576] Added nodes::

Please someone with permission set firefox18 as verified.
Verified fixed on FF 18 based on comment 20.
(In reply to MarioMi from comment #20)

Verified Fixed on Windows XP on FF 19b2 too.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.