Closed
Bug 471619
Opened 16 years ago
Closed 15 years ago
"ASSERTION: negative length" with moz-column, rtl, padding
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?] stack corruption)
Attachments
(2 files, 2 obsolete files)
###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file layout/generic/nsTextFrame.h, line 307
###!!! ASSERTION: Attempting to allocate excessively large array: 'Error', file nsTArray.cpp, line 69
###!!! ABORT: file nsTArray.cpp, line 70
In a nightly build, crashes dereferencing 0xc0000000 (note the leading 'c') and with a corrupt stack.
|
Reporter | |
Updated•16 years ago
|
Whiteboard: [sg:critical?]
|
Assignee | |
Comment 1•16 years ago
|
||
Assignee: nobody → mats.palmgren
|
|
Assignee | |
Comment 2•16 years ago
|
||
The WIP uncovers a bug in nsTextFrame::SetLength().
The call is SetLength(1) on the green frame, ie. changing
its next-in-flow frame's (blue) content offset from 50 to 51,
which is ok, but there is a non-fluid continuation (red) that
follows it that we don't adjust leading to the "negative length"
for the blue frame.
Attachment #355146 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 3•16 years ago
|
||
Here's the Steps To Reproduce the SetLength bug:
1. apply WIP
2. load the attached testcase (no crash)
3. zoom (CTRL++)
I tried to make an automatic crashtest for that but failed.
<html reftest-zoom="..."> didn't seem to trigger it, I also tried
loading the original testcase in an <iframe> and resizing that etc.
|
|
Assignee | |
Comment 4•16 years ago
|
||
FWIW, the original crash appears to overwrite the stack on x86_64 Linux.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:critical?] → [sg:critical?] stack corruption
|
|
Assignee | |
Comment 5•16 years ago
|
||
1. in nsFrame.cpp: fix the integer overflow
2. in BuildTextRunForFrames(): detect buffer overflow and bail. This is a
programming error for sure and thus should assert, but IIRC dveditz said
we should handle them if they had security implications. I think it's
motivated in this case. The m1b/m2b stuff is just to make the code more
readable, avoiding a lot of casting.
3. in SetLength(): adjust the offset for non-fluid continuations of our
next-in-flow.
You said in bug 406380 comment 5 that you didn't expect to cross a
non-fluid boundary in SetLength()... Is this what you meant?
nsTextFrame* f = static_cast<nsTextFrame*>(GetNextInFlow());
- if (!f)
- return;
+ if (!f) {
+ NS_ASSERTION(!GetNextContinuation(), "unexpected non-fluid continuation");
+ return;
I tested it and it does trigger the assertion. It can probably be
investigated in a separate bug though.
I also investigated bug 471594 (it has the "negative length" assertion)
in case it was also a problem with SetLength() somehow, but it appears to
be an independent problem.
Attachment #355915 -
Flags: superreview?(roc)
Attachment #355915 -
Flags: review?(roc)
(In reply to comment #5)
> You said in bug 406380 comment 5 that you didn't expect to cross a
> non-fluid boundary in SetLength()... Is this what you meant?
> nsTextFrame* f = static_cast<nsTextFrame*>(GetNextInFlow());
> - if (!f)
> - return;
> + if (!f) {
> + NS_ASSERTION(!GetNextContinuation(), "unexpected non-fluid continuation");
> + return;
>
> I tested it and it does trigger the assertion. It can probably be
> investigated in a separate bug though.
Not quite. We can call SetLength on the last frame of the fluid flow chain before a fixed-continuation boundary, but the offset of that fixed-continuation boundary should not be changing (i.e., fixed-continuation boundaries aren't changed by reflow --- that's why we call them fixed :-) ). So the assertion should be aLength == GetContentLength().
I don't see where in BuildTextRunForFrames you're bailing on buffer overflow.
The nsTextFrame::SetLength change seems wrong.
- f = static_cast<nsTextFrame*>(f->GetNextInFlow());
+ f = static_cast<nsTextFrame*>(f->GetNextContinuation());
If we're hitting a situation where we need to move a fixed-continuation boundary, something's gone very wrong elsewhere.
|
|
Assignee | |
Comment 8•16 years ago
|
||
> I don't see where in BuildTextRunForFrames you're bailing on buffer overflow.
Oops, this patch is old, I'll attach the correct one after some sleep ;-)
> The nsTextFrame::SetLength change seems wrong.
You can see the frame tree in attachment 355448 [details].
It doesn't seem wrong to me that if we need to take text from our
next-in-flow it may need to adjust some non-fluid continuations too.
In this case the next-in-flow (blue) has zero length.
|
|
Assignee | |
Updated•16 years ago
|
Attachment #355915 -
Attachment is obsolete: true
Attachment #355915 -
Flags: superreview?(roc)
Attachment #355915 -
Flags: review?(roc)
It does to me. Normally non-fluid continuations represent direction changes so pulling text across a non-fluid continuation boundary means we're changing its direction and screwing up bidi.
|
|
Assignee | |
Comment 10•16 years ago
|
||
Ok, that makes sense. The frame bidi properties are as follows:
SetLength(1) for 0x1be11d0:
Text(0)@0x1c28ad0 bidi:embed=1,base=1,char=10: mContentOffset=0 non-fluid
Text(0)@0x1be11d0 bidi:embed=2,base=1,char=0: mContentOffset=50 non-fluid
Text(0)@0x1ca5478 bidi:embed=1,base=1,char=10: mContentOffset=50 next-in-flow
Text(0)@0x14f1238 bidi:embed=2,base=1,char=0: mContentOffset=50 non-fluid
Text(0)@0x14f12a8 bidi:embed=1,base=1,char=10: mContentOffset=51 non-fluid
(they all have CSS direction RTL)
char=0 is eCharType_LeftToRight, 10 is eCharType_OtherNeutral.
|
|
Assignee | |
Comment 11•16 years ago
|
||
(In reply to comment #6)
> So the assertion should be aLength == GetContentLength().
That triggers when SetLength is called from nsTextFrame::Reflow and the
frame is incomplete.
Oh yeah, because the continuation doesn't exist yet :-(.
|
||
Comment 13•16 years ago
|
||
With the patch from bug 332655 there are a lot of assertions connected to unconstrained width, but no "negative length" and no crash.
Depends on: 332655
|
|
Reporter | |
Comment 14•15 years ago
|
||
Indeed, WFM.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WORKSFORME
|
||
Updated•9 years ago
|
Group: core-security → core-security-release
|
||
Updated•9 years ago
|
Group: core-security-release
|
|
Assignee | |
Updated•9 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•