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

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

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

For comparison, this is the corresponding frames with the patch.

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?

oh, comments about "our next in flow" should change to "our next-continuation".

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()).

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.

Comment on attachment 294933 [details] [diff] [review] Patch rev. 2 great, thanks!

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)

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.

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

Bug 411213 has a similar testcase and still asserts/crashes.

The testcase doesn't cause any assertions/crashes on the 1.8 branch.

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).