Crash with document.normalize, DOMCharacterDataModified mutation event

VERIFIED FIXED in mozilla7

Status

()

Core
DOM
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: sicking)

Tracking

(Blocks: 2 bugs, {crash, regression, testcase})

Trunk
mozilla7
x86
Mac OS X
crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
Created attachment 539981 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Comment 1

6 years ago
Created attachment 539982 [details]
stack trace
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?
Blocks: 659539
tracking-firefox7: --- → ?
Keywords: regression
Yes! We should do that!
Assignee: nobody → jonas
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.
Created attachment 541495 [details] [diff] [review]
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.
Attachment #541495 - Flags: review?(bzbarsky)
Comment on attachment 541495 [details] [diff] [review]
Patch to fix

r=me
Attachment #541495 - Flags: review?(bzbarsky) → review+
Created attachment 541760 [details] [diff] [review]
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.
Attachment #541760 - Flags: review?(bzbarsky)
Comment on attachment 541760 [details] [diff] [review]
interdiff

r=me
Attachment #541760 - Flags: review?(bzbarsky) → review+
Pushed to inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/2082bbcf2ce8
http://hg.mozilla.org/mozilla-central/rev/2082bbcf2ce8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Reporter)

Updated

6 years ago
Severity: normal → critical

Updated

6 years ago
tracking-firefox7: ? → ---
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.