664919 - Crash with document.normalize, DOMCharacterDataModified mutation event

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

We crash at here:

#2  0x0070d361 in nsGenericElement::Normalize (this=0x1b058240) at ../../../../mozilla/content/base/src/nsGenericElement.cpp:2791
2789              nsCOMPtr<nsIContent> sibling = GetChildAt(index + 1);
2790    
2791              if (sibling->NodeType() == nsIDOMNode::TEXT_NODE) {
(gdb) p sibling
$2 = {
  mRawPtr = 0x0
}

At this point index == 0 and count == 2.  We used to have a null-check on |sibling|, but that got removed in bug 659539 part 4....

Jonas, can we just defer mutation event firing till after normalize() is done so we don't have to worry about this sort of gunk?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes! We should do that!

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

hrm.. crap.. that's not trivial since we need to fire the DOMNodeRemoved events before removing any nodes :(

We could do a two-pass thing and first figure out which nodes are going to be removed, then do a second pass and actually remove them as we migrate their data to the nodes they are supposed to be merged with.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This can be simplified once we no longer have mutation events, but it'll be easier to simplify from this than from the current code anyway.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 541495 [details] [diff] [review]
Patch to fix

r=me

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: interdiff

The previous patch failed on try. I need to reset canMerge in more code paths (i.e. even when the current node is marked as to-be-merged) if there is no following sibling.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 541760 [details] [diff] [review]
interdiff

r=me

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Pushed to inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/2082bbcf2ce8

Comment 10

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/2082bbcf2ce8

Comment 11

[Trif Andrei-Alin[:AlinT]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

I runned the testcase on 7 beta build and Firefox did not crash.
Setting resolution to Verified Fixed.
Thanks.