Last Comment Bug 424276 - "ASSERTION: disconnected nodes" and "ASSERTION: invalid array index" with selection/range
: "ASSERTION: disconnected nodes" and "ASSERTION: invalid array index" with sel...
Status: RESOLVED FIXED
: [sg:critical?] post 1.8-branch
: assertion, crash, testcase, verified1.9.0.7
Product: Core
Classification: Components
Component: DOM: Traversal-Range
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9.1b2
Assigned To: Mats Palmgren [:mats]
: traversal-range
:
:
: 336383
  Show dependency treegraph
 
Reported: 2008-03-20 20:25 PDT by Jesse Ruderman
Modified: 2009-04-24 10:25 PDT (History)
16 users (show)
jst: blocking1.9.1-
jst: wanted1.9.1+
mtschrep: blocking1.9-
dveditz: blocking1.9.0.7+
samuel.sidler+old: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
bclary: in‑testsuite+
See Also:
Crash Signature:


Attachments
testcase (254 bytes, text/html)
2008-03-20 20:25 PDT, Jesse Ruderman
no flags Details
stack traces for the assertions (16.75 KB, text/plain)
2008-03-20 20:25 PDT, Jesse Ruderman
no flags Details
Patch rev. 1 (2.23 KB, patch)
2008-10-22 10:32 PDT, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
Patch rev. 2 (1.16 KB, patch)
2008-10-27 12:52 PDT, Mats Palmgren [:mats]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review
crashtest.diff (853 bytes, patch)
2008-11-02 08:33 PST, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-03-20 20:25:01 PDT
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.
Comment 1 Jesse Ruderman 2008-03-20 20:25:41 PDT
Created attachment 310913 [details]
stack traces for the assertions
Comment 2 Brandon Sterne (:bsterne) 2008-03-26 15:39:33 PDT
Given low severity, this can wait for a 1.9.0.x release.
Comment 3 Mike Schroepfer 2008-03-26 16:07:56 PDT
Given comment 2 moving to wanted list.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2008-09-24 16:28:34 PDT
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.
Comment 5 Jesse Ruderman 2008-09-24 17:01:06 PDT
I had set this as sg:low because it looked like a read (rather than a write) out of bounds.
Comment 6 Mats Palmgren [:mats] 2008-10-22 10:24:24 PDT
It's a SEGV crash with a Firefox debug build on Linux (64-bit).
Comment 7 Mats Palmgren [:mats] 2008-10-22 10:32:42 PDT
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).
Comment 8 Boris Zbarsky (:bz) 2008-10-22 11:16:24 PDT
Er....  But shouldn't we do something sane in this case with the return value instead of just using it?
Comment 9 Mats Palmgren [:mats] 2008-10-22 13:28:44 PDT
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?
Comment 10 Boris Zbarsky (:bz) 2008-10-22 13:44:03 PDT
Yeah, that sounds like a better idea than just hiding the problem.
Comment 11 Mats Palmgren [:mats] 2008-10-27 12:52:28 PDT
Created attachment 344954 [details] [review]
Patch rev. 2

Same patch without the nsSelection.cpp change.
Comment 12 Boris Zbarsky (:bz) 2008-10-28 11:40:13 PDT
Comment on attachment 344954 [details] [review]
Patch rev. 2

Please add a test.
Comment 13 Mats Palmgren [:mats] 2008-11-02 08:33:50 PST
Created attachment 345945 [details] [review]
crashtest.diff
Comment 14 Mats Palmgren [:mats] 2008-11-02 10:59:44 PST
http://hg.mozilla.org/mozilla-central/rev/6a229538f526

I'm holding the crashtest until 1.9.0.x is released with a fix.

-> FIXED
Comment 15 Jesse Ruderman 2008-11-03 11:48:16 PST
I filed bug 462897 on the remaining assertion.
Comment 16 Samuel Sidler (old account; do not CC) 2008-12-17 18:07:22 PST
Mats: Does this patch apply to 1.9.0? Can you please request approval if so?
Comment 17 Mats Palmgren [:mats] 2009-01-21 12:19:01 PST
Comment on attachment 344954 [details] [review]
Patch rev. 2

Yes, it applies and fixes the crash. (sorry, I forgot)
Comment 18 Daniel Veditz 2009-01-21 15:37:32 PST
Comment on attachment 344954 [details] [review]
Patch rev. 2

Approved for 1.9.0.7, a=dveditz for release-drivers.
Comment 19 Mats Palmgren [:mats] 2009-01-23 23:16:01 PST
Landed on CVS trunk for 1.9.0.7:
mozilla/content/base/src/nsContentUtils.cpp 	1.311
Comment 20 Daniel Veditz 2009-02-12 12:13:32 PST
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)
Comment 21 Al Billings [:abillings] 2009-02-13 12:11:05 PST
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).
Comment 22 Carsten Book [:Tomcat] 2009-02-14 08:56:26 PST
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
Comment 23 Bob Clary [:bc:] 2009-04-24 10:25:10 PDT
crash test added
http://hg.mozilla.org/mozilla-central/rev/1824b971b236

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