Last Comment Bug 664919 - Crash with document.normalize, DOMCharacterDataModified mutation event
: Crash with document.normalize, DOMCharacterDataModified mutation event
: 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)
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: ---

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)
bzbarsky: review+
Details | Diff | Review
interdiff (984 bytes, patch)
2011-06-24 12:34 PDT, Jonas Sicking (:sicking)
bzbarsky: review+
Details | Diff | 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);
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) 2011-06-16 21:48:34 PDT
Yes! We should do that!
Comment 4 Jonas Sicking (:sicking) 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) 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

Comment 7 Jonas Sicking (:sicking) 2011-06-24 12:34:32 PDT
Created attachment 541760 [details] [diff] [review]

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]

Comment 9 Jonas Sicking (:sicking) 2011-06-24 12:57:56 PDT
Pushed to inbound
Comment 10 Marco Bonardo [::mak] 2011-06-25 03:28:49 PDT
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.

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