Bugzilla@Mozilla – Bug 424276
"ASSERTION: disconnected nodes" and "ASSERTION: invalid array index" with selection/range
Last modified: 2009-04-24 10:25:10 PDT
Summon comment box
Created attachment 310912 [details] testcase Loading the testcase triggers: ###!!! ASSERTION: disconnected nodes: 'parents1.ElementAt(pos1) == parents2.ElementAt(pos2) || aDisconnected', file /Users/jruderman/trunk/mozilla/content/base/src/nsContentUtils.cpp, line 1570 ###!!! ASSERTION: invalid array index: 'i < Length()', file ../../dist/include/xpcom/nsTArray.h, line 317 Filing as security-sensitive because the "invalid array index" assertion is happening in nsTArray::ElementAt, which is not bounds-checked at runtime.
Created attachment 310913 [details] stack traces for the assertions
Given low severity, this can wait for a 1.9.0.x release.
Given comment 2 moving to wanted list.
On second thought, not blocking on this. Wanted 1.9.1 though. And this sounds like it could be sg:critical, given the out of bounds array access.
I had set this as sg:low because it looked like a read (rather than a write) out of bounds.
It's a SEGV crash with a Firefox debug build on Linux (64-bit).
Created attachment 344316 [details] [review] Patch rev. 1 The latter part of nsContentUtils::ComparePoints() doesn't make sense for disconnected nodes, so return early. The change in nsSelection.cpp is to allow comparing boundary points for ranges that may be disconnected, which I think is allowed so it shouldn't assert (but please correct me if I'm wrong).
Er.... But shouldn't we do something sane in this case with the return value instead of just using it?
Yeah, we should propagate 'disconnected' to the callers of CompareDOMPoints and make them deal with it properly. I'd like to fix the crash here first though and do that in a followup bug. Should I leave nsSelection.cpp as is and just let it assert then?
Yeah, that sounds like a better idea than just hiding the problem.
Created attachment 344954 [details] [review] Patch rev. 2 Same patch without the nsSelection.cpp change.
Comment on attachment 344954 [details] [review] Patch rev. 2 Please add a test.
Created attachment 345945 [details] [review] crashtest.diff
http://hg.mozilla.org/mozilla-central/rev/6a229538f526 I'm holding the crashtest until 1.9.0.x is released with a fix. -> FIXED
I filed bug 462897 on the remaining assertion.
Mats: Does this patch apply to 1.9.0? Can you please request approval if so?
Comment on attachment 344954 [details] [review] Patch rev. 2 Yes, it applies and fixes the crash. (sorry, I forgot)
Comment on attachment 344954 [details] [review] Patch rev. 2 Approved for 1.9.0.7, a=dveditz for release-drivers.
Landed on CVS trunk for 1.9.0.7: mozilla/content/base/src/nsContentUtils.cpp 1.311
The testcase does not crash 1.8 nor does the patched code seem to exist (unconnected ranges don't seem to exist until bug 409380)
The testcase doesn't crash 1.9.0.6 (non-debug). Tomcat, can you run the testcase with a debug 1.9.0.7 nightly? (If you have a debug 1.9.0.6 final, that would be good as well).
verified 1.9.0.7 using the testcase and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021407 Firefox/3.0.7pre i don't see the crash on this debug build, but ###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file /work/mozilla/builds/1.9.0/mozilla/content/base/src/nsContentUtils.cpp, line 1573
crash test added http://hg.mozilla.org/mozilla-central/rev/1824b971b236