668941 - "ASSERTION: frame crosses fixed continuation boundary" with rtl, pre-wrap, :first-letter

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file layout/base/nsLayoutUtils.cpp, line 4124

###!!! ASSERTION: frame crosses fixed continuation boundary: 'flowLength->mEndFlowOffset >= GetContentEnd()', file layout/generic/nsTextFrameThebes.cpp, line 643

The first bad revision is:
changeset:   99c270809649
user:        Simon Montagu
date:        Mon Jun 27 19:30:04 2011 +0300
summary:     Don't overshoot the end of the sibling chain when copying text for bidi resolution. Bug 665837, r=roc

Security-sensitive because there related testcases trigger scarier assertions, and because the regressor was security sensitive.

Comment 1

[Daniel Veditz [:dveditz]]

do regressions from such a simple fix mean we didn't really understand what was going on in bug 665837? Assuming the worst because frames scare me, although this assertion may simply mean there are frames we're not displaying when we should.

Maybe we need different macros for that kind of "assertion" so we can tell the difference.

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch

Comment 3

[Simon Montagu :smontagu]

(In reply to comment #1)
> do regressions from such a simple fix mean we didn't really understand what
> was going on in bug 665837?

No, the fix for bug 665837 exposed a regression (silly mistake) in one of the patches for bug 263359: in https://bugzilla.mozilla.org/attachment.cgi?id=498455&action=diff#a/layout/base/nsBidiPresUtils.cpp_sec3, I didn't take into account that adding a condition to the if would sometimes incorrectly activate the else-clause. That caused the situation which triggers the assert: a first-letter frame with a fluid continuation whose child text frame has a fixed continuation.

Comment 4

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

Comment on attachment 544746 [details] [diff] [review]
Patch

Review of attachment 544746 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 5

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/f7f3f4ad573c

Comment 6

[Asa Dotzler [:asa]]

Should this patch land on Aurora and or Beta? If so, can someone nominate?

Comment 7

[Simon Montagu :smontagu]

Comment on attachment 544746 [details] [diff] [review]
Patch

This fixes a regression from bug 263357 and bug 665837 which were checked in on Aurora, so the fix should go on Aurora too. Beta is unaffected.

Comment 8

[[:Cww]]

smontagu, the first bug you referenced was last touched (and marked INVA) over 4 years ago, probably a typo?

Comment 9

[Simon Montagu :smontagu]

(In reply to comment #8)
> smontagu, the first bug you referenced was last touched (and marked INVA)
> over 4 years ago, probably a typo?

Yes, typo for bug 263359

Comment 10

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-aurora/rev/ef4909389600

Comment 11

[LegNeato]

It looks like this regression was only on trunk and mozilla-aurora. If this doesn't apply to 1.9.2 please mark the status1.9.2 as unaffected. If it does, we'd like this in 1.9.2.21

Comment 12

[u279076]

qa- as no QA verification needed