Closed
Bug 482398
Opened 16 years ago
Closed 16 years ago
"ASSERTION: Unexpected frame types" with :first-letter, wrapping
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: bzbarsky)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files, 1 obsolete file)
437 bytes,
text/html
|
Details | |
14.25 KB,
patch
|
Details | Diff | Splinter Review |
###!!! 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
Assignee | ||
Comment 1•16 years ago
|
||
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.
Assignee | ||
Comment 2•16 years ago
|
||
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.
Assignee | ||
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
Patch for this will depend on bug 482592 in terms of applying.
Depends on: 482592
Assignee | ||
Comment 5•16 years ago
|
||
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
Assignee | ||
Comment 7•16 years ago
|
||
Attachment #366734 -
Attachment is obsolete: true
Assignee | ||
Comment 8•16 years ago
|
||
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
Closed: 16 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.
Assignee | ||
Comment 10•16 years ago
|
||
It would look like this patch, basically.
You need to log in
before you can comment on or make changes to this bug.
Description
•