Closed Bug 424276 Opened 14 years ago Closed 13 years ago
"ASSERTION: disconnected nodes" and "ASSERTION: invalid array index" with selection/range
254 bytes, text/html
16.75 KB, text/plain
1.16 KB, patch
|Details | Diff | Splinter Review|
853 bytes, patch
|Details | Diff | Splinter Review|
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.
Given low severity, this can wait for a 1.9.0.x release.
Given comment 2 moving to wanted list.
Flags: wanted1.9.0.x? → wanted1.9.0.x+
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
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
OS: Mac OS X → All
Hardware: PC → All
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.
Same patch without the nsSelection.cpp change.
Comment on attachment 344954 [details] [diff] [review] Patch rev. 2 Please add a test.
http://hg.mozilla.org/mozilla-central/rev/6a229538f526 I'm holding the crashtest until 1.9.0.x is released with a fix. -> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
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] [diff] [review] Patch rev. 2 Yes, it applies and fixes the crash. (sorry, I forgot)
Attachment #344954 - Flags: approval188.8.131.52?
Comment on attachment 344954 [details] [diff] [review] Patch rev. 2 Approved for 184.108.40.206, a=dveditz for release-drivers.
Attachment #344954 - Flags: approval220.127.116.11? → approval18.104.22.168+
Landed on CVS trunk for 22.214.171.124: 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)
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
The testcase doesn't crash 126.96.36.199 (non-debug). Tomcat, can you run the testcase with a debug 188.8.131.52 nightly? (If you have a debug 184.108.40.206 final, that would be good as well).
verified 220.127.116.11 using the testcase and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:18.104.22.168pre) 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
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.