Crash [@ nsTextFrameUtils::TransformText] with rtl, -moz-column, overflow: -moz-hidden-unscrollable

RESOLVED FIXED in mozilla1.9beta3

Status

()

Core
Layout
P2
critical
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla1.9beta3
assertion, crash, regression, rtl, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 291028 [details]
testcase (crashes Firefox when loaded)

Loading the testcase triggers 
"ASSERTION: Attempting to allocate excessively large array" in BuildTextRunsScanner::FlushFrames followed by a 
crash [@ nsTextFrameUtils::TransformText] with a corrupt stack.
Flags: blocking1.9?
(Reporter)

Updated

10 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 1

10 years ago
Created attachment 291071 [details]
stack for the assertion
(Assignee)

Comment 2

10 years ago
Created attachment 291072 [details]
Frame dumps

nsTextFrame::SetLength doesn't seem to deal with bidi continuations, 
it returns prematurely because GetNextInFlow() is null.
(Assignee)

Comment 3

10 years ago
Created attachment 291073 [details] [diff] [review]
Patch rev. 1

1. use GetNextContinuation() instead of GetNextInFlow()
2. add some assertions and minor cleanup

If you think the assertion in GetContentLength() is overkill, I can add some
assertions where we mess with mContentOffset instead...
Attachment #291073 - Flags: superreview?(roc)
Attachment #291073 - Flags: review?(roc)
(Assignee)

Comment 4

10 years ago
Created attachment 291074 [details]
Frame dumps #2

For comparison, this is the corresponding frames with the patch.
(Assignee)

Updated

10 years ago
Assignee: nobody → mats.palmgren
Flags: in-testsuite?
Keywords: regression
OS: Mac OS X → All
Hardware: PC → All
Comment on attachment 291073 [details] [diff] [review]
Patch rev. 1

+  PRInt32 GetContentLength() const { NS_ASSERTION(GetContentEnd() - mContentOffset >= 0, "negative length"); return GetContentEnd() - mContentOffset; }

This can go on two lines...

This is OK, but I never expected SetLength to have to cross a non-fluid continuation boundary (obviously), so why are we having to do that here?
Attachment #291073 - Flags: superreview?(roc)
Attachment #291073 - Flags: superreview+
Attachment #291073 - Flags: review?(roc)
Attachment #291073 - Flags: review+
oh, comments about "our next in flow" should change to "our next-continuation".

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
(Assignee)

Comment 7

10 years ago
Created attachment 294932 [details]
Frame dumps #3

I have tracked down where the error comes from:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsTextFrameThebes.cpp&rev=3.156&root=/cvsroot&mark=3163-3165,3167,3180,3195#3153

Since we have already hooked the new frame into the flow, line 3180 is
the same as "mContentOffset = mContentOffset". (Line 3195 is also a NOP).
The content offset for 'nextContinuation' is never adjusted though,
see attached trace (the pink lines are the content offset/length for the
frames at the end of nsContinuingTextFrame::Init()).
(Assignee)

Comment 8

10 years ago
Created attachment 294933 [details] [diff] [review]
Patch rev. 2

1. fixed nits
2. removed the NOPs and added a SetLength() call in
   nsContinuingTextFrame::Init() so that the content offset for the
   next-continuation(s) are updated when needed.
Attachment #291073 - Attachment is obsolete: true
Attachment #294933 - Flags: superreview?(roc)
Attachment #294933 - Flags: review?(roc)
Comment on attachment 294933 [details] [diff] [review]
Patch rev. 2

great, thanks!
Attachment #294933 - Flags: superreview?(roc)
Attachment #294933 - Flags: superreview+
Attachment #294933 - Flags: review?(roc)
Attachment #294933 - Flags: review+
(Assignee)

Comment 10

10 years ago
Created attachment 295235 [details]
Frame dumps #4 (375716-1-ref.html)

I landed this briefly but backed it out since it broke the reference
for bug 375716 (layout/reftests/bugs/375716-1-ref.html).
(The words in bold face should be RTL but were LTR on the second line.)

The problem was that SetLength() adjusted the offset (gave text to) a bidi
next-continuation of a different type.  This attachment shows the frame
tree before and after the SetLength() call at the end of
nsTextFrame::Reflow() of the blue frame:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsTextFrameThebes.cpp&rev=3.158&root=/cvsroot&mark=5546#5517
(only the first word fits on the first line, hence SetLength(6), but
since we have a non-fluid next-continuation (red) we gave it the text
even though it's LTR)
(Assignee)

Comment 11

10 years ago
Created attachment 295236 [details] [diff] [review]
Patch rev. 3

I think should leave SetLength() as is and adjust the content offset
for non-fluid continuations explicitly were needed.
This patch fixes the crash by doing that.

However, I do find it a bit odd that we have this frame situation at all
for the testcase. If you look at "frame dumps #3" you'll see that the
existing non-fluid continuation is on the Overflow-list and that we come
from nsBlockFrame::Reflow():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.917&root=/cvsroot&mark=912,915,924#847
I think it would be better if we can move the "DrainOverflowLines"
block before "ResolveBidi" block - this way we would have a less
complicated frame tree to deal with during bidi resolution (and it
would actually "fix" the crash since we avoid creating the next-in-flow
(0x1259da8) in this case).  I'll file a separate bug on this.
Attachment #294933 - Attachment is obsolete: true
Attachment #295236 - Flags: superreview?(roc)
Attachment #295236 - Flags: review?(roc)
Attachment #295236 - Flags: superreview?(roc)
Attachment #295236 - Flags: superreview+
Attachment #295236 - Flags: review?(roc)
Attachment #295236 - Flags: review+
(Assignee)

Comment 12

10 years ago
Filed bug 410857 on doing DrainOverflowLines() earlier.

mozilla/layout/generic/nsTextFrame.h 	3.20
mozilla/layout/generic/nsTextFrameThebes.cpp 	3.160
mozilla/layout/generic/crashtests/crashtests.list 	1.51 
mozilla/layout/generic/crashtests/406380.html 	1.1

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
(Reporter)

Updated

10 years ago
Depends on: 410967
(Reporter)

Comment 13

10 years ago
Bug 411213 has a similar testcase and still asserts/crashes.
(Reporter)

Comment 14

10 years ago
The testcase doesn't cause any assertions/crashes on the 1.8 branch.
Group: security
Flags: wanted1.8.1.x-

Comment 15

10 years ago
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Crash Signature: [@ nsTextFrameUtils::TransformText]
You need to log in before you can comment on or make changes to this bug.