385344 - Crash [@ gfxTextRun::ShrinkToLigatureBoundaries] with ::first-letter float: right and -moz-pre-wrap

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

The attached testcase started crashing after the new textrame got turned on.
Unfortunately, I can't give a stacktrace, because the nightly windows build isn't out yet.

Comment 1

[Martijn Wargers (dead)]

Ok, nightly build is out, here is the bp-id:
https://crash-reports.mozilla.com/reports/report/index/ff55c138-2017-11dc-bf1e-001a4bd43ed6
0  	gfxTextRun::ShrinkToLigatureBoundaries(unsigned int *,unsigned int *)
1 	gfxTextRun::BreakAndMeasureText(unsigned int,unsigned int,int,double,gfxTextRun::PropertyProvider *,int,double *,gfxFont::RunMetrics *,int,int *,unsigned int *)
2 	nsTextFrame::Reflow(nsPresContext *,nsHTMLReflowMetrics &,nsHTMLReflowState const &,unsigned int &)
3 	nsLineLayout::ReflowFrame(nsIFrame *,unsigned int &,nsHTMLReflowMetrics *,int &)
4 	nsInlineFrame::ReflowInlineFrame(nsPresContext *,nsHTMLReflowState const &,nsInlineFrame::InlineReflowState &,nsIFrame *,unsigned int &)
5 	nsInlineFrame::ReflowFrames(nsPresContext *,nsHTMLReflowState const &,nsInlineFrame::InlineReflowState &,nsHTMLReflowMetrics &,unsigned int &)
6 	nsInlineFrame::Reflow(nsPresContext *,nsHTMLReflowMetrics &,nsHTMLReflowState const &,unsigned int &)
7 	nsLineLayout::ReflowFrame(nsIFrame *,unsigned int &,nsHTMLReflowMetrics *,int &)
8 	nsBlockFrame::ReflowInlineFrame(nsBlockReflowState &,nsLineLayout &,nsLineList_iterator,nsIFrame *,LineReflowStatus *)
9 	nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState &,nsLineLayout &,nsLineList_iterator,int *,LineReflowStatus *,int)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

There's two problems here:

1) after a first-letter/first-line frame has reflowed, its next-in-flow may need to update its mContentOffset. The mContentOffset is adjusted lazily in Reflow() (which is pretty annoying, but that's the way we've always done it). Anyway, after we've done that, we need to ensure that its textrun is reconstructed, because its text may have a different style from the first-letter/first-line. So this patch marks text frames which are part of first-letter/first-line and forces their next-in-flows to reconstruct textruns every time they reflow.

2) another problem is in nsTextFrame::AddInlinePrefWidthForFlow. For preformatted whitespace we loop over the text, breaking it at newline characters. We were looping over all the text associated with this text frame and its next-in-flows, but that's wrong when the text is in first-letter/first-line, because this textrun may not have text for the next-in-flows. So in this patch we just use flowEndInTextRun instead and do everything using textrun offsets instead of content offsets.

3) I also changed nsTextFrame::AddInlineMinWidthForFlow to use the textrun's text when looking for preformatted newlines, because that's simpler.

4) I also noticed that placeholder frames were breaking up textruns. We actually don't want this, a placeholder is essentially invisible so we should be able to build textruns across placeholders.

Comment 3

[Simon Montagu :smontagu]

I probably won't be able to get to this until after the weekend (i.e. Saturday night PDT)

Comment 4

[Martijn Wargers (dead)]

Attachment: testcase2

I tried the patch in my debug build, but it seems that it causes the attached testcase2 to crash.
Just before I crash, I see this assertion:
###!!! ASSERTION: Attempting to allocate excessively large array: 'Error', file
c:/mozilla/mozilla/xpcom/build/nsTArray.cpp, line 68

Comment 5

[Martijn Wargers (dead)]

Hmm, maybe I can see it also in regularly nightly builds, although more seldom.
It seems to crash when I've minimized it for a little while (30s?) and then maximize it again. 

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 269343 [details] [diff] [review]
fix

>+// Set when the frame's text may have a different style from its in-flow
>+// brethren (e.g. the frame is in first-letter or first-line), so text runs
>+// may need to be reconstructed when the frame's content offset/length changes.
>+// We set this on the frame that has in first-letter or first-line, but not
>+// in-flow siblings outside first-letter or first-line.

Nit: Personally I like your use of "brethren", but I think non-native speakers (and some native speakers!) will find it obscure, and it's not clear whether it's synonymous with "siblings" or not.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Martijn, that crash is another bug, can you please file it? I debugged it a bit and the problem is that nsBidiPresUtils's bidi resolution doesn't understand floated first-letters at all...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I checked this in.

Comment 9

[Martijn Wargers (dead)]

Ok, I filed the testcase2 crashing issue as bug 385751.

Comment 10

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071905 Minefield/3.0a7pre

Comment 11

[Mats Palmgren (inactive)]

Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe39fce24d66

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fe39fce24d66