If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

"ASSERTION: Unexpected frame types" with :first-letter, wrapping

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 366493 [details]
testcase

###!!! ASSERTION: Unexpected frame types: '(frame1->GetType() == nsGkAtoms::tableFrame && frame2->GetType() == nsGkAtoms::tableOuterFrame) || (frame1->GetType() == nsGkAtoms::tableOuterFrame && frame2->GetType() == nsGkAtoms::tableFrame) || frame1->GetType() == nsGkAtoms::fieldSetFrame || (frame1->GetParent() && frame1->GetParent()->GetType() == nsGkAtoms::fieldSetFrame)', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 8002

Also, the letter 'c' appears in the wrong place.

This assertion was added in bug 480979 part 6:
http://hg.mozilla.org/mozilla-central/rev/159168c61b02
Hey, good news and bad news.

Bad news is it asserts.

Good news is that the assert and the wrong place bug are the same issue, I would hope, and the wrong place bug is a regression from Fx2 to Fx3.  We just finally caught it.

That bug was intruduced between 2007-08-02-04 and 2007-08-03-04.  Bonsai query: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-08-02+04&maxdate=2007-08-03+04&cvsroot=%2Fcvsroot

I blame bug 386014 at first blush, as something that touched the relevant code.
And I bet the reason the wrapping matters is that prevSibling->GetParent() is before the break while our parentFrame is after the break.  Which really means that we're using the wrong parentFrame, of course.
Yeah, this is kinda sad.  On entry to ContentInserted, the prevSibling is the textframe for "B" and the parentFrame is the Letter frame around it.

Then the RemoveLetterFrames call not only removes all those letter frames, but merges the textframes for A and B (which used to have different parents).  However, the inline that used to be the Letter frame's parent (and which we assigned to parentFrame before removing the Letter frames) is still there.  But it is not in fact the parent of the prevSibling we now compute (which is the textframe for "A B").

We used to blithely ignore this and just insert in parentFrame with the bogus prevSibling, parented the new frame to parentFrame, but then stuck it in the child list of parentFrames prev-in-flow.  Luckily during the next reflow it got pushed to the next line again and hence ended up in the correct parent.
Blocks: 386014
Patch for this will depend on bug 482592 in terms of applying.
Depends on: 482592
Created attachment 366734 [details] [diff] [review]
Proposed fix

Basic idea here is to just stop trying to be clever about first-letter and properly reget our prevsibling and parent.
Assignee: nobody → bzbarsky
Attachment #366734 - Flags: superreview?(dbaron)
Attachment #366734 - Flags: review?(dbaron)
Attachment #366734 - Flags: superreview?(dbaron)
Attachment #366734 - Flags: superreview+
Attachment #366734 - Flags: review?(dbaron)
Attachment #366734 - Flags: review+
Comment on attachment 366734 [details] [diff] [review]
Proposed fix

r+sr=dbaron; sorry about the delay
Created attachment 374389 [details] [diff] [review]
Merged to tip
Attachment #366734 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/af71fccc01be

It's probably too late in the 1.9.1 cycle to take this there... though maybe it makes sense to fix the correctness problem?
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I really don't know this code well enough to have an opinion on that, nor do I really know what a fix for just the correctness problem would look like.
It would look like this patch, basically.
You need to log in before you can comment on or make changes to this bug.