Closed Bug 668941 Opened 8 years ago Closed 8 years ago

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

Categories

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

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox5 --- unaffected
firefox7 + fixed
firefox8 --- fixed
status1.9.2 --- unaffected

People

(Reporter: jruderman, Assigned: smontagu)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical?] [landed m-c 7/12][qa-])

Attachments

(2 files)

Attached file 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.
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.
Whiteboard: [sg:critical?]
Attached patch PatchSplinter Review
Assignee: nobody → smontagu
Attachment #544746 - Flags: review?(roc)
(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 on attachment 544746 [details] [diff] [review]
Patch

Review of attachment 544746 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #544746 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/f7f3f4ad573c
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Should this patch land on Aurora and or Beta? If so, can someone nominate?
Whiteboard: [sg:critical?] → [sg:critical?] [landed m-c 7/12]
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.
Attachment #544746 - Flags: approval-mozilla-aurora?
Attachment #544746 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
smontagu, the first bug you referenced was last touched (and marked INVA) over 4 years ago, probably a typo?
(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
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
qa- as no QA verification needed
Whiteboard: [sg:critical?] [landed m-c 7/12] → [sg:critical?] [landed m-c 7/12][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.