288560 - Columns: Hang/freeze in this case using float:right and -moz-column-count:2

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
Basically, the testcase consists of this:
<div style="height:1px;-moz-column-count:2;">
<span style="float:right;border:1px solid red;"></span>
</div>

This causes the freeze/hang. Also, memory seems to increase steadily by 0.5MB/s.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Robert, this is probably also something for you.

Comment 3

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

Attachment: fix

This is a tough one. We're trying to lay out a float and there simply isn't
enough	available height for it to fit anywhere, so we just keep creating more
columns in the hope that it will eventually fit, and of course that doesn't
help.

What we have to do is what we do elsewhere, which is determine when moving the
content to the next page/column won't give it any more space, and if so, force
it to fit where it is. "fit where it is" is tricky for floats though, because
the space manager is involved. This patch detects force-fit situations and
forces floats to fit by temporarily relaxing the height constraint imposed by
nsBlockBandData. That seems to work well.

To fully fix this bug, though, we have to work a bit harder. Columns need to
set the mIsTopOfPage flag so that the float doesn't get NS_FRAME_IS_TRUNCATED
in its reflow status, which triggers nsLineLayout to try to push the line
anyway.

Comment 4

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

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

r+sr=dbaron, although I wonder if the overloading of mIsTopOfPage will cause
problems when columns and pagination are combined.

Comment 5

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

Looking again at the patch, there's a significant problem ...

+  nsSize space = mSpace;
+  if (aRelaxHeightConstraint) {
+    space.height = NS_UNCONSTRAINEDSIZE;
+  }

but 'space' isn't used, we just keep using mSpace. I'll fix that and retest...

Comment 6

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

Attachment: testcase

Yeah. The change to nsColumnSetFrame alone fixes Martijn's original testcase,
but the erroneous patch here doesn't fix this testcase.

Comment 7

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

I think the overloading of mIsTopOfPage won't cause problems. The only
potentially dangerous situation I can think of is when a column set starts right
at the bottom of a page, so the column set doesn't have mIsTopOfPage set, but
the first lines of its columns do, so those lines are not marked as truncated
even when they overflow the height constraint. But the column set itself will be
marked as truncated and pushed to the next page.

Comment 8

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

Attachment: fix #2

This patch fixes both Martijn's testcase and the testcase I just posted. It's
basically identical to the previous patch except it makes the float-forcefit
code actually work by using 'space' instead of 'mSpace' as I'd intended. I also
made another change in nsBlockFrame::PushLine so that the forcefit parameter to
PlaceBelowCurrentLineFloats uses the IsAdjacentToTop value from *before* we
placed the line, not after (when it would almost always be false).

Comment 9

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

Comment on attachment 182033 [details] [diff] [review]
fix #2

wrong patch

Comment 10

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

Attachment: fix #2

right patch

Comment 11

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

Comment on attachment 182034 [details] [diff] [review]
fix #2

fixes a hang in some column testcases (which can also be triggered in print
preview). Should have no effect on normal layout.

Comment 12

[chris hofmann]

Comment on attachment 182034 [details] [diff] [review]
fix #2

a=chofmann

Comment 13

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

checked in