390762 - Crash [@ nsFrameManager::UnregisterPlaceholderFrame] with -moz-column and float

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox within a few seconds)

###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/generic/nsFrame.cpp, line 5591

###!!! ASSERTION: No placeholder for out-of-flow?: 'placeholderFrame', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9556

###!!! ASSERTION: null param unexpected: 'aPlaceholderFrame', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 510

Crash [@ nsFrameManager::UnregisterPlaceholderFrame]

The testcase is pretty similar to the one in bug 386266.  The stack signature is the same, too, but this is a null deref instead of an 0xdddddddd thing.

Comment 1

[Eli Friedman]

Attachment: Patch

This is bad.  Really bad.  We really ought to simplify frame construction.

Comment 2

[Eli Friedman]

(And I probably ought to back out the patch to bug 386266; it was the wrong fix.)

Comment 3

[fantasai]

> +    do {

I'm guessing you're not using DoRemoveFrame because it doesn't MarkSameSpaceManagerLinesDirty(). Are other parts of this function also missing calls to MarkSameSpaceManagerLinesDirty(), e.g. if we remove a block that contains floats?

> +      nsIFrame* continuation = curFrame->GetNextContinuation();

I didn't think floats could have non-fluid continuations. We use GetNextInFlow everywhere else, so unless I'm wrong about that, we probably want to use GetNextInFlow here, too.

Comment 4

[Eli Friedman]

(In reply to comment #3)
> > +    do {
> 
> I'm guessing you're not using DoRemoveFrame because it doesn't
> MarkSameSpaceManagerLinesDirty().

No, the code doesn't use DoRemoveFrame because it can't remove out-of-flows.

> Are other parts of this function also missing
> calls to MarkSameSpaceManagerLinesDirty(), e.g. if we remove a block that
> contains floats?

We call it for in-flows and floats, and abs-pos frames have their own frame manager; I have no clue what cases make us call RemoveFrame with nsGkAtoms::nextBidi, but I'm assuming it doesn't happen with block children.  Hmm, it actually looks like we don't call it for continuing blocks, but we have more serious issues in cases like that (see bug 390897).

> > +      nsIFrame* continuation = curFrame->GetNextContinuation();
> 
> I didn't think floats could have non-fluid continuations. We use GetNextInFlow
> everywhere else, so unless I'm wrong about that, we probably want to use
> GetNextInFlow here, too.

Floats can't have non-fluid continuations, but if we did give them non-fluid continuations, we wouldn't want to miss destroying them.

Comment 5

[fantasai]

DoRemoveFrames calls DoRemoveOutOfFlowFrame for out-of-flows.
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#5077

Abspos frames wouldn't need to call MarkSameSpaceManagerLinesDirty because their floats can't affect content outside the abspos frame.

Comment 6

[Eli Friedman]

(In reply to comment #5)
> DoRemoveFrames calls DoRemoveOutOfFlowFrame for out-of-flows.
> http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#5077

Ah, true... but there isn't really much point, considering that all it ends up doing is calling RemoveFloat.

Comment 7

[fantasai]

Recursively, yes.

Comment 8

[Eli Friedman]

Oh, right... I guess I'll use it, then.

Comment 9

[Eli Friedman]

Attachment: Patch v2

Okay, that works... I'll file a followup about making us not use recursion for destroying continuations.  (Recursion obviously has some bad effects.)

It would be nice if we could somehow assert that we aren't orphaning any frames, but it seems difficult to do.

Comment 10

[fantasai]

Your new patch looks really good except.. doesn't it forget to mark the float continuations' parents dirty?

Comment 11

[Eli Friedman]

How am I supposed to know which frames to mark dirty?

Comment 12

[fantasai]

Ok.. I think I understand this a bit better now.

You want to mark dirty all lines that would be affected by the float and its continuations. The existing code uses MarkSameSpaceManagerLinesDirty, which walks up the ancestor chain until it finds the ancestor with a space manager for this float, and marks all the descendant lines dirty (though not all lines would be affected). You want to do this for all the continuations as well, which you did in your first patch. Marking the float's parent dirty like you do in v2 causes all the parent's lines to be marked dirty, but it won't mark any descendant blocks' lines dirty, and it won't affect lines on any later blocks also affected by the float. AFAICT DoRemoveFrame does not mark any lines dirty, so nothing is happening for the floats continuations, either, even though they are properly removed.

So, as a summary, your first patch probably did a more proper job of marking things dirty. We have an existing function DoRemoveFrame which does something similar, and maybe we should be using that. But it doesn't mark things dirty properly. Maybe it should. I really don't know. Maybe roc can provide some advice here...

As a conclusion, I think we have too many ill-defined remove functions in nsBlockFrame.cpp and the whole situation is rather confusing. ._.

Comment 13

[Eli Friedman]

Does that mean the first patch was better?

Comment 14

[fantasai]

Comment on attachment 275645 [details] [diff] [review]
Patch v2

AFAICT, patch 1 was a correct fix for this bug, whereas patch 2 isn't. Additionally, it seems like we may have a problem in RemoveFloat: it should probably be calling MarkSameSpaceManagerLinesDirty. If that's the case then you wouldn't have to do that explicitly here.
I guess I'll just give r=fantasai on patch 1 and let the roc deal with whether we need to fix RemoveFloat or use DoRemoveFrame.

Comment 15

[Eli Friedman]

Checked in; not sure how to add this to automated testing.

Comment 16

[Jesse Ruderman]

Crashtest checked in.