389583 - Asserts on redoing totally empty line.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

I've got two testcases that cause asserts about redoing a totally empty line, both derived from a testcase from fantasai. There are two separate issues and I have a patch that fixes both of them.

Comment 1

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

Attachment: testcase #1

Comment 2

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

Attachment: testcase #2

Comment 3

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

Attachment: fix testcase #1

This is a very simple one, it should be obvious.

Comment 4

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

Attachment: fix testcase #2

Here the situation is that there is a float extending to the bottom of the column and we try to place another float, which doesn't fit in the remaining width. During nsBlockReflowState::FlowAndPlaceFloat, we call nsBlockReflowState::GetAvailableSpace repeatedly; it finally returns a band rectangle with height zero at the bottom of the column which is considered *not* to be adjacent to the existing float.

I think this behaviour is acceptable (and changing it seems risky), but we need to change this assertion so that it doesn't fire in that case.

Comment 5

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

(I think this behaviour is acceptable because it's conceptually indeterminate whether a zero-height band which is at a float's top or bottom edge is "adjacent to" that float or not.)

Comment 6

[Eli Friedman]

(In reply to comment #2)
> Created an attachment (id=273837) [details]
> testcase #2

This testcase appears to render fine without any assertions on my box; maybe the patch to Bug 387201?

Comment 7

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

Maybe, but I don't see how. You're still returning a zero-height band with no floats in it, as far as I can tell.

Comment 8

[Jesse Ruderman]

Text of the assertion, for searching purposes:

###!!! ASSERTION: redo line on totally empty line: 'aState.IsImpactedByFloat()', file mozilla/layout/generic/nsBlockFrame.cpp, line 3356

Comment 9

[Jesse Ruderman]

Testcase 1 also triggers:

###!!! ASSERTION: bad prev-in-flow ancestor chain: 'nsLayoutUtils::IsProperAncestorFrame(prevBlock, fpif)', file mozilla/layout/generic/nsBlockFrame.cpp, line 4346

Comment 10

[Mats Palmgren (inactive)]

(In reply to comment #9)
> Testcase 1 also triggers:
> ###!!! ASSERTION: bad prev-in-flow ancestor chain:

I think this assertion is a recent regression, last 7 days or so.

Comment 11

[Mats Palmgren (inactive)]

(bug 301844 / bug 389776 covers the new assertion)

Comment 12

[fantasai]

Attachment: original testcase

for the record

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 273840 [details] [diff] [review]
fix testcase #1

sr=mats

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 273844 [details] [diff] [review]
fix testcase #2

>Index: layout/generic/nsBlockFrame.cpp
>   if (LINE_REFLOW_REDO_NEXT_BAND == lineReflowStatus) {
>     // This happens only when we have a line that is impacted by
>     // floats and the first element in the line doesn't fit with
>     // the floats.
>     //
>     // What we do is to advance past the first float we find and
>     // then reflow the line all over again.
>-    NS_ASSERTION(aState.IsImpactedByFloat(),
>-                 "redo line on totally empty line");

The comment talks about *a line* that is impacted by floats,
maybe the assertion was meant to check that?
That is, shouldn't we assert "aLine->IsImpactedByFloat()" instead?

Comment 15

[Mats Palmgren (inactive)]

BTW, the layout for testcase #2 is:

+--------+       |        |       +--------+       |        |
|        |       +--------+       |        |       +--------+
|        |                        |        |       
|        |                        |        |       
|        |                        |        |       
|        |                        |        |       

shouldn't it be like this instead:

+--------+       |        |       |        |
|        |       +--------+       |        |
|        |       +--------+       |        |
|        |       |        |       +--------+                 
|        |       |        |       	                      
|        |       |        |       	                       

Comment 16

[Eli Friedman]

The correct rendering is actually more like:

+--------++--------+ |        ||        |
|        ||        | |        ||        |
|        ||        | |        ||        |
|        ||        | +--------++--------+
|        ||        |
|        ||        |
|        ||        |

The two floats should be able to fit next to each other.

Note that that's actually the rendering I was getting in my build until recently... not sure what changed it.

I think we shouldn't screw around with the assertion until the testcase is fixed.

Comment 17

[Mats Palmgren (inactive)]

> The two floats should be able to fit next to each other.

But the column width is less than the float margin-box width.  Since the
second float doesn't fit next to the first float I would expect it to be
moved below.  (which is what we appear to be trying to do, although we
over do it IMO)
Increasing the column width slightly gives the layout you're suggesting.

Comment 18

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

(In reply to comment #14)
> (From update of attachment 273844 [details] [diff] [review])
> >Index: layout/generic/nsBlockFrame.cpp
> >   if (LINE_REFLOW_REDO_NEXT_BAND == lineReflowStatus) {
> >     // This happens only when we have a line that is impacted by
> >     // floats and the first element in the line doesn't fit with
> >     // the floats.
> >     //
> >     // What we do is to advance past the first float we find and
> >     // then reflow the line all over again.
> >-    NS_ASSERTION(aState.IsImpactedByFloat(),
> >-                 "redo line on totally empty line");
> 
> The comment talks about *a line* that is impacted by floats,
> maybe the assertion was meant to check that?
> That is, shouldn't we assert "aLine->IsImpactedByFloat()" instead?

aState.IsImpactedByFloat() is checking the right thing, which is whether the current band has at least one float in it. aLine->IsImpactedByFloat might be equivalent but I'd have to check to see when exactly that gets set.

> shouldn't it be like this instead:

Yes. The rendering is still wrong after these patches, and I believe the rendering should be as you (Mats) suggest --- not what Eli suggests; the float border-boxes are each 110px wide, so they can't fit next to each other in a 200px wide column.

The problem with the rendering is that when we reflow the second float, aState.mY (the current Y coordinate) is 0, i.e. we're at the top of the second column. So we reflow it there and the float breaks so the first piece has height 250px. But the float doesn't fit into the band containing the last piece of the first float. We try the next band, where it fits horizontally, but we don't reflow the float again so now it doesn't fit vertically! And it gets pushed to the next column. The most obvious way to fix this would be to reflow a float again if we move it down and it doesn't fit vertically anymore. But that's more than I want to do in this bug.

Comment 19

[Eli Friedman]

I think you're misinterpreting the float reflow code; see http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockReflowState.cpp#831.

Comment 20

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

oops, I missed that. (dbaron's comment is wrong, therefore.) There must be some other explanation for the broken rendering, then.

Comment 21

[Eli Friedman]

(In reply to comment #19)
> I think you're misinterpreting the float reflow code; see
> http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockReflowState.cpp#831.
> 

Oops, you're right... we actually should be reflowing, but we aren't.

Comment 22

[Eli Friedman]

Comment on attachment 273844 [details] [diff] [review]
fix testcase #2

Actually I think our current behavior on this testcase is intentional... it has the effect of avoiding breaking floats if we don't have to.

r+ assuming you fix up the comment before the assertion to mention this situation.

Comment 23

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

I guess that is true. OK, thanks.

Comment 24

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

Comment on attachment 273840 [details] [diff] [review]
fix testcase #1

a1.9=dbaron

Comment 25

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

checked in attachment 273840 [details] [diff] [review]

Comment 26

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

Comment on attachment 273844 [details] [diff] [review]
fix testcase #2

just moves an assert, zero risk

Comment 27

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

Comment on attachment 273844 [details] [diff] [review]
fix testcase #2

a1.9=dbaron

Comment 28

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

checked that in too.