424276 - "ASSERTION: disconnected nodes" and "ASSERTION: invalid array index" with selection/range

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

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]

Created attachment 310913 [details]
stack traces for the assertions

Comment 2

[Brandon Sterne (:bsterne)]

Given low severity, this can wait for a 1.9.0.x release.

Comment 3

[Mike Schroepfer]

Given comment 2 moving to wanted list.

Comment 4

[Johnny Stenback (:jst)]

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]

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)]

It's a SEGV crash with a Firefox debug build on Linux (64-bit).

Comment 7

[Mats Palmgren (:mats)]

Created attachment 344316 [details] [diff] [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 [:bzbarsky, bz on IRC]]

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)]

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 [:bzbarsky, bz on IRC]]

Yeah, that sounds like a better idea than just hiding the problem.

Comment 11

[Mats Palmgren (:mats)]

Created attachment 344954 [details] [diff] [review]
Patch rev. 2

Same patch without the nsSelection.cpp change.

Comment 12

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 344954 [details] [diff] [review]
Patch rev. 2

Please add a test.

Comment 13

[Mats Palmgren (:mats)]

Created attachment 345945 [details] [diff] [review]
crashtest.diff

Comment 14

[Mats Palmgren (:mats)]

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]

I filed bug 462897 on the remaining assertion.

Comment 16

[Samuel Sidler (old account; do not CC)]

Mats: Does this patch apply to 1.9.0? Can you please request approval if so?

Comment 17

[Mats Palmgren (:mats)]

Comment on attachment 344954 [details] [diff] [review]
Patch rev. 2

Yes, it applies and fixes the crash. (sorry, I forgot)

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 344954 [details] [diff] [review]
Patch rev. 2

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 19

[Mats Palmgren (:mats)]

Landed on CVS trunk for 1.9.0.7:
mozilla/content/base/src/nsContentUtils.cpp 	1.311

Comment 20

[Daniel Veditz [:dveditz]]

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]]

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]]

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:]]

crash test added
http://hg.mozilla.org/mozilla-central/rev/1824b971b236