317278 - ASSERTION: bad float placement: 'NS_SUCCEEDED(rv)' - Creating a circular frame list, this is very bad.

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, I get two assertions with that testcase in my debug build, these are the warnings/assertions I get:
WARNING: aFrame is already associated with a region, file c:/mozilla/mozilla/lay
out/generic/nsSpaceManager.cpp, line 814
###!!! ASSERTION: bad float placement: 'NS_SUCCEEDED(rv)', file c:/mozilla/mozil
la/layout/generic/nsBlockReflowState.cpp, line 1005
WARNING: aFrame is already associated with a region, file c:/mozilla/mozilla/lay
out/generic/nsSpaceManager.cpp, line 814
###!!! ASSERTION: bad float placement: 'NS_SUCCEEDED(rv)', file c:/mozilla/mozil
la/layout/generic/nsBlockReflowState.cpp, line 1005
###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aN
extSibling', file c:/mozilla/mozilla/layout/generic/nsIFrame.h, line 695
###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aN
extSibling', file c:/mozilla/mozilla/layout/generic/nsIFrame.h, line 695

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Basically, we're calling AddRectRegion() twice on the same space manager with the same frame.  That really shouldn't be happening, as I understand this code.

Comment 3

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

The problem is that we've got this unwrappable span containing a float. So we reflow the entire span, including the float, and then discover that the span doesn't fit and has to be pushed. But we don't remove the pushed float from mBelowCurrentLineFloats, so it gets placed after that line, and then it gets reflowed and placed again on the next line. In the testcase, you can see the space bogusly taken up by the float after the first line.

In general when we split a line, if there are placed floats after the split, we need to unwind the float state, including removing any such floats from aState.mBelowCurrentLineFloats and the line's float cache, and removing any such floats from the space manager. Ick!

Normally there aren't any placed floats because we will detect that text doesn't fit and split the line before we try to place floats after that text.

Comment 4

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

Perhaps we could remember "break here!" at the right content+offset, and then reflow the line again, forcing a break at the given position. We already allow redoing line reflow (to move lines down vertically when they don't fit next to floats), this would extend that.

We could use that here to reflow the line again if split-off floats are detected, and avoid pulling up the split-off new line.

We could also use this to fix the textframe mess that handles words containig multiple textframes, e.g., "<span>Hello Kit</span>ty". Currently the span textframe does nasty, complicated lookahead through the (not very well formed, because they haven't been reflowed yet) frame tree measuring text to see whether the complete word will fit. Instead, it could record "I can break here" at the space, just assume everything fits and return from Reflow(). Then if we find "ty" doesn't fit, we reflow the line again forcing it to break at the last remembered break point.

Does this sound like a good idea? The textframe changes would not be wanted on branch, of course, but the rest could be branch-worthy if we find we need to fix this bug on branch. It's not clear that we do.

Comment 5

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

Attachment: fix

This fixes it using the redo approach described above. The redo machinery here doesn't support breaking the line at an arbitary point like I want to do for textframes, that can be added later. It just reflows the current line again and preserves the current line break by refusing to pull frames up from the next line.

This probably also fixes some bugs with the existing redo code not undoing floats properly.

Comment 6

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

Are the nsFloatCacheList/nsFloatCacheFreeList changes needed?  Do we ever store nsFloatCacheFreeList objects as nsFloatCacheList objects?  Should we prevent that from being possible instead of adding virtual functions?

The idea of this patch is to place the text first, and then the float it contains, right?  Or the opposite (which violates the CSS spec, IIRC)?

Comment 7

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

(In reply to comment #6)
> Are the nsFloatCacheList/nsFloatCacheFreeList changes needed?

I needed an implementation of DeleteAll for nsFloatCacheFreeList so I could call aState.mBelowCurrentLineFloats.DeleteAll(), so I refactored the code to create one.

> Do we ever store
> nsFloatCacheFreeList objects as nsFloatCacheList objects?

I don't think so.

> Should we prevent that from being possible instead of adding virtual
> functions?

You mean make the inheritance relationship private, or break it? I guess we could do that. Is that the way you want to go?

> The idea of this patch is to place the text first, and then the float it
> contains, right?  Or the opposite (which violates the CSS spec, IIRC)?

This patch is not supposed to change any layout behaviour except for the incorrect thing going on here. All that I'm trying to do here is guarantee that if a span is pushed to the following line, any floats in it also move to the following line, in particular floats that we may have already placed in the current line.

Comment 8

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

Seems like private inheritance (with a few inline forwarding functions) would probably be better than adding virtual functions.

Comment 9

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

OK, I'll do that.

Comment 10

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

There remains a bug in a case like this:

<p><span style="white-space:pre"><div style="float:left; height:1em; width:90%">hello Hello hello</span>

where we place the float, which fits, but then the rest of the span doesn't fit. We don't push the span because it's the first thing on the line, so it ends up overflowing the available width. What we really should do (I think) is convert the placed float to a below-current-line float so we end up with the span on one line and the float below it. I think I want to deal with that in another bug.

Comment 11

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

Attachment: fix

updated with float list changes

Comment 12

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

Comment on attachment 224819 [details] [diff] [review]
fix

nsFloatCacheFreeList::DeleteAll should probably call
nsFloatCacheList::DeleteAll and then set mTail to null (rather than
being a separate implementation).

Could you make what you implemented as nsFloatCacheList::Remove
protected (and with another name), and have an inline version with void return (calling the protected method) for the public API?  Returning the previous element is highly unintuitive.

nsFloatCacheList::DeleteAll would probably be slightly better off if it
iterated over a temporary variable rather than assigning to mHead since
the compiler can optimize it better.  (Just don't forget to make mHead
null in the end.)

You detached the comment about line layout allocation from the stack
allocation itself.  Could you move those back together?

+ // If floats have been placed that have been pushed to the new line,
Fix by s/that have/whose placeholders have/

The names LINE_REFLOW_REDO and LINE_REFLOW_REDO_FOR_FLOAT confuse me a
good bit.  I think I'd prefer something like:
 LINE_REFLOW_REDO_FOR_FLOAT -> LINE_REFLOW_REDO_BELOW_FLOAT
 LINE_REFLOW_REDO -> LINE_REFLOW_REDO_NO_PULL
(I'm not attached to these either, so feel free to improve.)  This
change would require renaming didRedoForFloat too.

At first I was a little uncomfortable with popping the space manager for
LINE_REFLOW_REDO_FOR_FLOAT too, but I suppose it makes sense and there's
probably a similar nasty bug in that case.

It seems like this patch may make it easier to fix bug 50630.

Comment 13

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

How about
LINE_REFLOW_REDO_NEXT_BAND
LINE_REFLOW_REDO_NO_PULL
?

Comment 14

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

Even better.

Comment 15

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

checked in.

Comment 16

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

Did we want this in 1.8.1, or is it not needed there for some reason?

Comment 17

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

I backed this out because it regressed Tp on btek and probably on luna.

Comment 18

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

I think this is a bit risky for 1.8.1 unless/until we get significant bake time. Furthermore, I think we should land the fix for bug 282173 (RemoveBuildFloatList) on the branches, and if we do that, then this bug should not be a crasher on the branches.

Comment 19

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

Attachment: fix

This is the patch I had checked in, updated to trunk and with review comments addressed.

Comment 20

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

Attachment: updated patch

Hopefully this will improve things. I noticed that in many cases the new call to PushState/PopState on the space manager was forcing a dynamic allocation because we've already got one state pushed (e.g., while we do an unconstrained-width reflow of the line). This patch is the same as what I checked in except that I've altered PushState/PopState to save and restore to a state object, which can be stack allocated --- and is, everywhere --- so we completely eliminate dynamic allocations. It also simplifies bookkeeping and removes the need to call DiscardState explicitly.

Comment 21

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

Relanded.

Comment 22

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

This appears to have regressed btek again. I'll back it out after one more cycle.

Comment 23

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

After another cycle, it looks like this was just noise on btek, so I'm leaving it in. Yay!

Comment 24

[Jesse Ruderman]

Bug 342322 has a testcase that triggers the "Can only remove a singleton element" assertion added in this bug.

Comment 25

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

When I checked in the fix for 237085, I somehow backed this out :-( :-(.

I'll check it in again