Last Comment Bug 664919 - Crash with document.normalize, DOMCharacterDataModified mutation event
: Crash with document.normalize, DOMCharacterDataModified mutation event
Status: VERIFIED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla7
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on:
Blocks: 325861 326633 659539
  Show dependency treegraph
 
Reported: 2011-06-16 21:05 PDT by Jesse Ruderman
Modified: 2013-12-27 14:29 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (534 bytes, text/html)
2011-06-16 21:05 PDT, Jesse Ruderman
no flags Details
stack trace (3.00 KB, text/plain)
2011-06-16 21:06 PDT, Jesse Ruderman
no flags Details
Patch to fix (12.15 KB, patch)
2011-06-23 14:16 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bzbarsky: review+
Details | Diff | Splinter Review
interdiff (984 bytes, patch)
2011-06-24 12:34 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bzbarsky: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-06-16 21:05:26 PDT
Created attachment 539981 [details]
testcase (crashes Firefox when loaded)
Comment 1 Jesse Ruderman 2011-06-16 21:06:37 PDT
Created attachment 539982 [details]
stack trace
Comment 2 Boris Zbarsky [:bz] 2011-06-16 21:23:29 PDT
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) PTO Until July 5th 2011-06-16 21:48:34 PDT
Yes! We should do that!
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-16 21:53:44 PDT
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) PTO Until July 5th 2011-06-23 14:16:53 PDT
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.
Comment 6 Boris Zbarsky [:bz] 2011-06-23 14:41:16 PDT
Comment on attachment 541495 [details] [diff] [review]
Patch to fix

r=me
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-24 12:34:32 PDT
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.
Comment 8 Boris Zbarsky [:bz] 2011-06-24 12:37:58 PDT
Comment on attachment 541760 [details] [diff] [review]
interdiff

r=me
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-24 12:57:56 PDT
Pushed to inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/2082bbcf2ce8
Comment 10 Marco Bonardo [::mak] 2011-06-25 03:28:49 PDT
http://hg.mozilla.org/mozilla-central/rev/2082bbcf2ce8
Comment 11 Trif Andrei-Alin[:AlinT] 2011-08-24 02:10:16 PDT
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.

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