Closed Bug 482398 Opened 11 years ago Closed 11 years ago

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


(Core :: Layout, defect)

Not set





(Reporter: jruderman, Assigned: bzbarsky)


(Blocks 2 open bugs)


(Keywords: assertion, testcase)


(2 files, 1 obsolete file)

Attached file 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:
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:

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
Attached patch Proposed fix (obsolete) — Splinter Review
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
Attached patch Merged to tipSplinter Review
Attachment #366734 - Attachment is obsolete: true

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?
Closed: 11 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.