398453 - Hang on print preview with overflow: scroll, float: left and page-break-after

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which hangs with current trunk build on print preview.
It doesn't hang with branch builds, so it seems like a regression.
I can look for the regression range, if wanted.

Comment 1

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

Probably a layout bug, in reality

Comment 2

[Daniel Holbert [:dholbert]]

Looks like we're doing some nscoord_MAX arithmetic. I get these right at the beginning of the hang:

nsBlockReflowContext: Frame(span)(1)@0xb16903f4 metrics=60,1073741820!
nsBlockReflowContext: HTMLScroll(div)(1)@0xb168ff44 metrics=400,1073743020!

Note that 1073743020 = nscoord_MAX - 4

Comment 3

[Daniel Holbert [:dholbert]]

> Note that 1073743020 = nscoord_MAX - 4

Oops, I misread the number -- it's actually this:

  1073743020 = nscoord_MAX + 1196

Comment 4

[Daniel Holbert [:dholbert]]

Ah, here's what I meant:

First message(span):  height = 1073741820 = nscoord_MAX + 4
Second message(div):  height = 1073743020 = nscoord_MAX + 1196

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> First message(span):  height = 1073741820 = nscoord_MAX + 4

D'oh, one final obvious correction... s/+/-/ on that one. :)
Ok, done making stupid typos for now.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This patch catches two instances of nscoord_MAX math and fixes the hang.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 286098 [details] [diff] [review]
patch v1

>-        nscoord newY = aLine->mBounds.YMost();
>+        nscoord newY = PR_MIN(nscoord_MAX, aLine->mBounds.YMost());

Note: In this case, we had 
  aLine->mBounds.y = 1200
  aLine->mBounds.height = nscoord_MAX

so YMost returned nscoord_MAX + 1200, and that got us into trouble.

Rather than this PR_MIN check, maybe we should just fix the XMost() and YMost() function to use the NSCoordSaturatingAdd (or a simpler add + check) internally...?   That'd be cleaner in some ways (namely, it'd make YMost and XMost return their true "most" values in nscoord_MAX cases, rather than dangerous nscoords near to the max).  It'd also prevent lots of future instances of this sort of bug, in cases where a width or height is nscoord_MAX and we call XMost / YMost.

But maybe the overhead of that would be bad, since we call XMost / YMost a fair amount?

Comment 8

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

the second part of the patch is OK, but I really think we should not be setting the x/y of an nsRect to something that's not a genuine coordinate.

Comment 9

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

(ditto for width/height)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch v2

> but I really think we should not be setting
> the x/y of an nsRect to something that's not a genuine coordinate.

roc and I discussed this more in IRC -- the nsRect unconstrained height comes from an nsHTMLReflowMetrics unconstrained height, and roc said that nsHTMLReflowMetrics probably shouldn't have unconstrained heights either.

So, this new patch goes back to the first time that we set a nsHTMLReflowMetrics height to be nscoord_MAX (which incidentally is the same area of the second part of the last patch), and it sets the height to be 0 (rather than availableHeight) if availableHeight is unconstrained.

This change is in a nsPageBreakFrame, which I don't think does very much (its Reflow function is about ten lines long), so I think that giving it a height of 0 in this case is fine.  I'm not 100% sure yet though, and it's possible that some other height value makes more sense.  Gonna investigate a bit more tomorrow.

Comment 11

[Daniel Holbert [:dholbert]]

Notes on patch v2:
 Pros:
  - fixes the hang
  - In the absence of better height information, giving a frame 0 height is definitely safer than giving it an (invalid) NS_UNCONSTRAINEDSIZE height. (particularly when we start doing arithmetic on the height)

 Cons:
  - This patch would make nsPageBreakFrames somewhat useless inside of "overflow: scroll" regions. (Though that's better than having them be useless *and* cause hangs, as before)  Since the nsPageBreakFrame would get zero height in that situation, it wouldn't actually force the page to break.

I don't know how big of an issue that con is... is "page-break-after inside overflow:scroll" a use case that would come up much?

I think in an ideal world, we'd *really* like to do something like this:  ignore the unconstrained availableHeight, and look all the way up to our nsPageFrame, get its height and our y-position within that page, and then set the pageBreakFrame's height to [page height - myYPosition].  I'm not sure that'd always work, though, and it may be a bit hack-ish...

roc, what do you think?

Comment 12

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

We don't support breaking inside of an overflow:scroll/auto area, so this is fine. This whole approach to forced page breaks is stupid anyway, and will probably go away one way or another.

Comment 13

[Martijn Wargers (dead)]

Daniel, I think the patch can be checked in, now.

Comment 14

[Daniel Holbert [:dholbert]]

patch v2 checked in.

/cvsroot/mozilla/layout/generic/nsPageFrame.cpp,v  <--  nsPageFrame.cpp
new revision: 3.176; previous revision: 3.175

Comment 15

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007110805 Minefield/3.0b2pre