380012 - [FIX]Assertions and hang with :first-line

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Jesse Ruderman]

Attachment: Hangs Firefox when loaded, about 50% of the time

###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', layout/generic/nsHTMLReflowState.cpp, line 425
###!!! ASSERTION: not a floated frame: 'aChildFrame->GetStyleDisplay()->IsFloating()', layout/base/nsCSSFrameConstructor.cpp, line 1753
###!!! ASSERTION: not in child list: 'found', layout/base/nsCSSFrameConstructor.cpp, line 1777
###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aNextSibling', layout/base/../generic/nsIFrame.h, line 750
###!!! ASSERTION: loop in frame list.  This will probably hang soon.: 'Error', layout/generic/nsFrameList.cpp, line 580

Before reducing, I was getting sg:critical crashes about 10% of the time.

Perhaps another regression from bug 372550?

Comment 1

[Jesse Ruderman]

The crashes had nsAbsoluteContainingBlock::DestroyFrames calling 0xdddddddd, fwiw.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Welcome to the hell that is the definition of first-line in CSS!  The WG can't even decide what it should do or how it should work...

The problem here is that the placeholder for the abs pos node is a child of the first-line.  So when we figure out the style parent for that node we come up with the first-line, and it inherits its display from there.  That's broken.  The rest is corollaries.  I'm guessing that any fix that leaves text inside first-line working is ok, right?  :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Evil hack

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: The right diff

It's hacky, but we really do want a different behavior for placeholders and text frames, unfortunately... :(

I could also give placeholders a different anon box pseudo and special-case it here if you're prefer, I guess.  This seemed simpler.

Comment 8

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

(In reply to comment #2)
> The problem here is that the placeholder for the abs pos node is a child of the
> first-line.  So when we figure out the style parent for that node we come up
> with the first-line, and it inherits its display from there.  That's broken. 

Why is that broken?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Why is that broken?

Because that's not what happened during initial frame tree construction... and it gets the rest of our code confused somehow.  In particular, we end up with a frame in an absolute-list whose position struct has mPosition == NS_STYLE_POSITION_STATIC, and things go downhill from there.

Comment 10

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

Comment on attachment 264083 [details] [diff] [review]
The right diff

>+    if (!parentPseudo ||
>+        (!nsCSSAnonBoxes::IsAnonBox(parentPseudo) &&
>+         aChildPseudo != nsGkAtoms::placeholderFrame)) {

Comment that nsGkAtoms::placeholderFrame is a bogus aChildPseudo passed in from nsPlaceholderFrame to trigger this behavior?

And change the comment above the loop to add "(and, for placeholder frames, pseudo-elements as well)".


I still don't understand why placeholders specifically are parented differently than other things inside pseudo-elements.  Does ReParentStyleContext not touch them?

(And I still don't know what makes us reparent things correctly inside the first-letter frame for the part other than the first letter, or the line frame for the part other than the first line.)

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I still don't understand why placeholders specifically are parented differently
> than other things inside pseudo-elements.

I'm trying to recall a debugging session from two months ago here, but the basic problem was that reparenting didn't actually reconstruct frames based on the new style information, and then later on our style data didn't match the frame tree, so we didn't clean things up properly, leading to problems.  I can re-debug this, I guess, if you want more details.  I should have saved them.  :(

There are other known issues with the setup, but for in-flows kids we don't depend on the style to properly tear down the frame tree...

Comment 12

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

Comment on attachment 264083 [details] [diff] [review]
The right diff

OK, and then this will make reparenting not change the style data for out-of-flows?  I suppose that's sensible.

r+sr=dbaron, with the changes I mentioned above in comment 10.

Comment 13

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

But please also add a comment explaining that we need this so that ReParentStyleContext doesn't lead to the style data being out of sync with the frames.

Then again, if we don't process change lists from ReParentStyleContext, couldn't there be a lot of other problems?

Comment 14

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

Should there be a followup bug on doing change list processing in ReParentStyleContext?  Or should we do that instead of this patch?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> OK, and then this will make reparenting not change the style data for
> out-of-flows?

Exactly.

> couldn't there be a lot of other problems?

Probably, yes...

> Should there be a followup bug on doing change list processing in
> ReParentStyleContext? 

I think what we should really do is maybe implement Hixie's proposal for this stuff: have a separate parent for inherit and reset structs in this case, so that reparenting can only change properties that inherit by default....  I do think we should have a followup bug on making first-line work sanely.  Will file if we don't have one already.

In general, processing change lists from ReParentStyleContext wouldn't work on its own, since said reparenting can happen during reflow, when we can't process things... and since reconstructing a frame may not place it inside the first-line during the frame construction, but then put it there during reflow, etc.

I'll make the comment changes.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed, with the comment changes.  Filed bug 388529.

Comment 17

[Jesse Ruderman]

I can get branch to assert but not hang or crash, so I'm making this bug report public.

Comment 18

[Jesse Ruderman]

Crashtest checked in.

Comment 19

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre - i was not able to get a Firefox while using the Testcases

--> Verified fixed