Closed Bug 424276 Opened 15 years ago Closed 14 years ago

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


(Core :: DOM: Core & HTML, defect, P1)






(Reporter: jruderman, Assigned: MatsPalmgren_bugz)



(4 keywords, Whiteboard: [sg:critical?] post 1.8-branch)


(4 files, 1 obsolete file)

Attached file 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.
Whiteboard: [sg:low?]
Flags: blocking1.9?
Given low severity, this can wait for a 1.9.0.x release.
Given comment 2 moving to wanted list.
Flags: tracking1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Flags: wanted1.9.0.x?
Flags: tracking1.9+
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
Target Milestone: --- → mozilla1.9.1
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.
Assignee: nobody → bent.mozilla
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Priority: P3 → P1
Whiteboard: [sg:low?] → [sg:critical?]
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).
Assignee: bent.mozilla → mats.palmgren
Severity: normal → critical
Keywords: crash
OS: Mac OS X → All
Hardware: PC → All
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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).
Attachment #344316 - Flags: superreview?(bzbarsky)
Attachment #344316 - Flags: review?(bzbarsky)
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.
Attached patch Patch rev. 2Splinter Review
Same patch without the nsSelection.cpp change.
Attachment #344316 - Attachment is obsolete: true
Attachment #344954 - Flags: superreview?(bzbarsky)
Attachment #344954 - Flags: review?(bzbarsky)
Attachment #344316 - Flags: superreview?(bzbarsky)
Attachment #344316 - Flags: review?(bzbarsky)
Comment on attachment 344954 [details] [diff] [review]
Patch rev. 2

Please add a test.
Attachment #344954 - Flags: superreview?(bzbarsky)
Attachment #344954 - Flags: superreview+
Attachment #344954 - Flags: review?(bzbarsky)
Attachment #344954 - Flags: review+
Attached patch crashtest.diffSplinter Review

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

Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
I filed bug 462897 on the remaining assertion.
Flags: blocking1.9.0.6?
Mats: Does this patch apply to 1.9.0? Can you please request approval if so?
Flags: blocking1.9.0.6?
Flags: blocking1.9.0.7?
Comment on attachment 344954 [details] [diff] [review]
Patch rev. 2

Yes, it applies and fixes the crash. (sorry, I forgot)
Attachment #344954 - Flags: approval1.9.0.7?
Flags: blocking1.9.0.7? → blocking1.9.0.7+
Comment on attachment 344954 [details] [diff] [review]
Patch rev. 2

Approved for, a=dveditz for release-drivers.
Attachment #344954 - Flags: approval1.9.0.7? → approval1.9.0.7+
Landed on CVS trunk for
mozilla/content/base/src/nsContentUtils.cpp 	1.311
Keywords: fixed1.9.0.7
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)
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
The testcase doesn't crash (non-debug). 

Tomcat, can you run the testcase with a debug nightly? (If you have a debug final, that would be good as well).
verified using the testcase and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: 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
Group: core-security
crash test added
Flags: in-testsuite? → in-testsuite+
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.