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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Patch for this will depend on bug 482592 in terms of applying.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Basic idea here is to just stop trying to be clever about first-letter and properly reget our prevsibling and parent.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 366734 [details] [diff] [review]
Proposed fix

r+sr=dbaron; sorry about the delay

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 8

[Boris Zbarsky [:bzbarsky]]

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?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

It would look like this patch, basically.