425981 - "ASSERTION: What happened here?" with -moz-column, floating first-letter

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: What happened here?: 'containingBlock == GetFloatContainingBlock(parentFrame)', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9642

If I remove the last line from boom(), I get the assertion in bug 417902 instead, so the two bugs might be related.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I would have thought that the patch in bug 413048 fixes this, but we still split on floated first-letter, apparently.  This looks pretty bad to me.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (crashes firefox)

Here's a modified version of the testcase that crashes firefox.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 341525 [details]
testcase 2 (crashes firefox)

(In reply to comment #0)
> If I remove the last line from boom(), I get the assertion in bug 417902

Actually, that's one of the changes I did to make testcase 2... and I do get that other assertion before the crash in testcase 2, instead of the assertion this bug was filed on.

--> Reposting testcase 2 on bug 417902, and obsoleting this version, as it probably makes more sense there.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't see us splitting a floated first-letter here.

Basically the problem is that the nsFrameConstructorState we pass into RecoverLetterFrames in nsCSSFrameConstructor::ContentAppended is the one we construct for the appended frames. In this testcase, that state's float containing block is the last block of the column set, so RecoverLetterFrames actually ends up constructing the float for the letter frames in that last block instead of in the first block where it belongs.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it. There are quite a few places where we need to use the insertion point instead of the parent frame for locating the first-line/first-letter.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  I thought we had code in RemoveLetterFrames to remove from the first continuation of the block, no?

And the same for RecoverLetterFrames, as far as I can tell.

Furthermore, I don't see why the containing block of insertionPoint would necessarily be a first-continuation block...

What am I missing?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is that the float is constructed using the passed-in aState, whose float containing block is always the block where content is being appended or inserted.

But you're right, this patch isn't right. I'll have to rework it to construct the state inside CreateLetterFrame which is where we actually know where the letter frame is going to be created.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  That makes sense, then.  And yeah, sounds like just changing CreateLetterFrame for the floating case is the way to go.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

This should be better

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 355887 [details] [diff] [review]
fix v2

Yeah, much better.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed b4d826e10c0c

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 355887 [details] [diff] [review]
fix v2

>@@ -11286,17 +11286,17 @@ nsCSSFrameConstructor::ProcessChildren(n
> 
>   // restore the incoming pseudo frame state
>   aState.mPseudoFrames = priorPseudoFrames;
> 
>   NS_ASSERTION(!aAllowBlockStyles || !aFrame->IsBoxFrame(),
>                "can't be both block and box");
> 
>   if (haveFirstLetterStyle) {
>-    rv = WrapFramesInFirstLetterFrame(aState, aContent, aFrame, aFrameItems);
>+    rv = WrapFramesInFirstLetterFrame(aContent, aFrame, aFrameItems);
>   }
>   if (haveFirstLineStyle) {
>     rv = WrapFramesInFirstLineFrame(aState, aContent, aFrame, aFrameItems);
>   }
> 
>   nsIContent *badKid;
>   if (aFrame->IsBoxFrame() &&
>       (badKid = AnyKidsNeedBlockParent(aFrameItems.childList))) {

This chunk doesn't apply on the 1.9.1 branch.

Comment 13

[Daniel Holbert [:dholbert]]

I'm presuming the above-mentioned chunk should apply to this code on the 1.9.1 branch:
http://mxr.mozilla.org/mozilla1.9.1/source/layout/base/nsCSSFrameConstructor.cpp#11380
11380   // restore the incoming pseudo frame state
11381   aState.mPseudoFrames = priorPseudoFrames;
11382 
11383   if (aParentIsBlock) {
11384     if (aState.mFirstLetterStyle) {
11385       rv = WrapFramesInFirstLetterFrame(aState, aContent, aFrame, aFrameItems);
11386     }
11387     if (aState.mFirstLineStyle) {
11388       rv = WrapFramesInFirstLineFrame(aState, aContent, aFrame, aFrameItems);
11389     }
11390   }
11391 
11392   return rv;

Seems reasonable, because I think that's the last remaining call to WrapFramesInFirstLetterFrame, and it's just the function signature that we're tweaking.  The code around that call must've been restructured in a different mozilla-central patch.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: fix v2, backported to 1.9.1

Here's fix v2, with comment 12 / comment 13 addressed so that it applies to the 1.9.1 branch.  I verified that this builds correctly and fixes the assertion on the 1.9.1 branch.

Comment 15

[Daniel Holbert [:dholbert]]

Pushed "fix v2, backported to 1.9.1" to 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bcdafacca3b1

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Shouldn't the "status1.9.1" field be updated to ".4-fixed" then?

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Marking as verified for 1.9.1 based on passing test.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to comment #16)
> Shouldn't the "status1.9.1" field be updated to ".4-fixed" then?

Sorry -- I should have added the "fixed1.9.1" keyword back when this landed in comment 15 (January 2009).

This was fixed long before the 1.9.1 final release.  So if ".4-fixed" means "this was fixed in 1.9.1.4 and not fixed before that", then that'd be an incorrect setting for that field.  However, if it just means "this was fixed sometime before 1.9.1.4", then that'd be fine.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to comment #18)
> (In reply to comment #16)
> > Shouldn't the "status1.9.1" field be updated to ".4-fixed" then?
> 
> Sorry -- I should have added the "fixed1.9.1" keyword back when this landed in
> comment 15 (January 2009).

Oh, haha -- looks like this bug's "History" page says that I *did* add that keyword. :)  (I assumed from Comment 16 that I hadn't.)

Gavin says in IRC that the current value of "verified1.9.1" is fine. So I think this bug is ok as-is.