864444 - [multicol] Test in layout/generic/crashtests/479938-1.html has contentBottom that overflows nscoord

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Scott Johnson (:jwir3)]

When running the test layout/generic/crashtest/479938-1.html, I see the following:

> *** Doing column reflow pass: mLastBalanceHeight=2280, mColMaxHeight=2280, RTL=0
> , mBalanceColCount=2, mColWidth=17310, mColGap=960
> *** Reflowing child #0 0x189d4d0: availHeight=2280
> *** Reflowed child #0 0x189d4d0: status = 6, desiredSize=17310,60 CarriedOutBottomMargin=0
> *** NEXT CHILD ORIGIN.x = 18270
> *** Reflowing child #1 0x189ef38: availHeight=1073741824
> *** Reflowed child #1 0x189ef38: status = 0, desiredSize=17310,0 CarriedOutBottomMargin=0
> *** DONE PASS feasible=1
> *** nsColumnSetFrame::Reflow balancing knownInfeasible=2279 knownFeasible=2280
> [New Thread 0x7fff4ffff700 (LWP 10780)]
> WARNING: Overflowed nscoord_MAX in conversion to nscoord: 'aValue <= nscoord_MAX', > file ../../dist/include/nsCoord.h, line 83
> WARNING: Overflowed nscoord_MAX in conversion to nscoord: 'aValue <= nscoord_MAX', > file ../../dist/include/nsCoord.h, line 83
> WARNING: Overflowed nscoord_MAX in conversion to nscoord: 'aValue <= nscoord_MAX', > file ../../dist/include/nsCoord.h, line 83
> WARNING: Overflowed nscoord_MAX in conversion to nscoord: 'aValue <= nscoord_MAX', > file ../../dist/include/nsCoord.h, line 83
> *** nsColumnSetFrame::ChooseColumnStrategy: numColumns=2, colWidth=17310, expectedWidthLeftOver=0, colHeight=2280, colGap=960
> *** Doing column reflow pass: mLastBalanceHeight=2280, mColMaxHeight=2280, RTL=0
, mBalanceColCount=2, mColWidth=17310, mColGap=960
> *** Reflowing child #0 0x189d4d0: availHeight=2280
> WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 131
> WARNING: Overflowed nscoord_MAX in conversion to nscoord height: file ../../dist/include/nsRect.h, line 144
> WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 131
> WARNING: Overflowed nscoord_MAX in conversion to nscoord height: file ../../dist/include/nsRect.h, line 144
> WARNING: Overflowed nscoord_MAX in conversion to nscoord height: file ../../dist/include/nsRect.h, line 144
> WARNING: Overflowed nscoord_MAX in conversion to nscoord height: file ../../dist/include/nsRect.h, line 144
> nsBlockReflowContext: Block(div)(0)@0x189dbb0 metrics=1073741824,1073742964!
> WARNING: Overflowed nscoord_MAX in conversion to nscoord height: file ../../dist/include/nsRect.h, line 144
> WARNING: Overflowed nscoord_MAX in conversion to nscoord height: file ../../dist/include/nsRect.h, line 144
> *** Reflowed child #0 0x189d4d0: status = 6, desiredSize=17310,60 CarriedOutBottomMargin=0

> Breakpoint 2, nsColumnSetFrame::ReflowChildren (this=this@entry=0x189d5a0, aDesiredSize=..., aReflowState=..., 
     aStatus=@0x7fffffffa0b4: 6, aConfig=..., aUnboundedLastColumn=aUnboundedLastColumn@entry=true, 
    aBottomMarginCarriedOut=0x7fffffff9f14, aColData=...)
    at /home/sjohnson/Source/mozilla-central/mozilla/layout/generic/nsColumnSetFrame.cpp:621
> 621	      if ((contentBottom > aReflowState.mComputedMaxHeight ||
> (gdb) p contentBottom
> $4 = 1073742964

This definitely seems incorrect to me, and is causing assertions to fail that should be true when the patch to bug 857324 is pushed. 

I don't think contentBottom should ever exceed NS_INTRINSICSIZE for a given column.

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b864444

I found that nsRect clamps the sizes (in the worst case of overflow) to nscoord_MAX, which is larger than NS_INTRINSICSIZE. So, I clamp the contentBottom calculation to NS_INTRINSICSIZE in this situation, so that we use the correct logic if this happens.

Also, I added back the assertion that was removed in bug 857324. 

Try server run:
https://tbpl.mozilla.org/?tree=Try&rev=3fda01f0d388

Comment 2

[Mats Palmgren (inactive)]

> ... to nscoord_MAX, which is larger than NS_INTRINSICSIZE.

Actually, they are all the same value:
gfx/src/nsSize.h:#define NS_MAXSIZE nscoord_MAX
layout/generic/nsHTMLReflowState.h:#define NS_UNCONSTRAINEDSIZE NS_MAXSIZE
layout/generic/nsHTMLReflowState.h:#define NS_INTRINSICSIZE    NS_UNCONSTRAINEDSIZE
layout/generic/nsHTMLReflowState.h:#define NS_AUTOHEIGHT       NS_UNCONSTRAINEDSIZE
layout/generic/nsHTMLReflowState.h:#define NS_AUTOMARGIN       NS_UNCONSTRAINEDSIZE
layout/generic/nsHTMLReflowState.h:#define NS_AUTOOFFSET       NS_UNCONSTRAINEDSIZE

so, if I understand this correctly, what usually happens when
contentBottom > nscoord_MAX we'll enter this block when the (max-)height is auto:

      if ((contentBottom > aReflowState.mComputedMaxHeight ||
           contentBottom > aReflowState.ComputedHeight()) &&
           aConfig.mBalanceColCount < INT32_MAX) {
        // We overflowed vertically, but have not exceeded the number of
        // columns. We're going to go into overflow columns now, so balancing
        // no longer applies.
        aColData.mHasExcessHeight = true;
      }

By clamping 'contentBottom' we'll avoid that (and thus not reach the
assertion).

> I don't think contentBottom should ever exceed NS_INTRINSICSIZE
> for a given column.

nscoords should never exceed nscoord_MAX, true.  But adding code
to handle it makes the code harder to understand and is ultimately
an exercise in futility.  I think we should only address it where
it causes hangs, crashes or performance problems.
What the resulting layout is when you're using "padding:67108863pc"
etc doesn't matter -- it's undefined (even per CSS specs iirc).

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 746422 [details] [diff] [review]
b864444

I think the height of the block with padding:67108863pc was clamped to
nscoord_MAX and then something got added to that for 'contentBottom'.
At the point of the assertion it's nscoord_MAX + 19px.
(also, at that point 'childContentBottom' is nscoord_MAX + 57px)

I think the current code deals with nscoord_MAX overflows as it is,
so I don't see why we should change it.

Comment 4

[Scott Johnson (:jwir3)]

(In reply to Mats Palmgren [:mats] from comment #3)
> Comment on attachment 746422 [details] [diff] [review]
> b864444
> 
> I think the height of the block with padding:67108863pc was clamped to
> nscoord_MAX and then something got added to that for 'contentBottom'.
> At the point of the assertion it's nscoord_MAX + 19px.
> (also, at that point 'childContentBottom' is nscoord_MAX + 57px)
> 
> I think the current code deals with nscoord_MAX overflows as it is,
> so I don't see why we should change it.

Fair enough. I'll leave it be and close this as WONTFIX.