381074 - Hang with display: table-caption, large padding and left float inside it

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which hangs current trunk Mozilla builds on load.
It doesn't hang in branch builds.
If wanted, I could find out a regression range for this.

Comment 1

[Martijn Wargers (dead)]

Ok, so this regressed between 2007-02-06 and 2007-02-08, which is when the patch for bug 177805 went in, so I guess this is somehow a regression from bug 177805.

Comment 2

[Eli Friedman]

It's hanging in nsSpaceManager::GetBandAvailableSpace in the first while loop; somehow there's a cycle in the band list.

Comment 3

[Eli Friedman]

It's probably getting confused by the negative padding...

Comment 4

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

Negative padding shouldn't get beyond the CSS parser -- it's a syntax error.

Comment 5

[Eli Friedman]

(In reply to comment #4)
> Negative padding shouldn't get beyond the CSS parser -- it's a syntax error.

The issue isn't the parsed value; it's the computed value.

According to the DOM inspector, the style rule says padding-top: 1e+8px, but the computed value is padding-top: -1.78957e+7px.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Did we overflow somewhere or something?

Comment 7

[Eli Friedman]

(In reply to comment #6)
> Did we overflow somewhere or something?

I think so.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er, of course.  We have 1e8 pixels, 60 app units per pixel, so 6e9 app units.  That doesn't fit in 32 bits, of course, much less the 30 bits nscoord actually has.

Not sure what we want to do here, short of going to a float nscoord...

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch (bounds-check in NSFloatPixelsToAppUnits)

The conversion that causes the nscoord overflow is happening here:

 nsRuleNode.cpp:297 (in SetCoord)
  nsRuleNode.cpp:215 (in CalcLength)
   nsPresContext.cpp:490 (in CSSPixelsToAppUnits)
    nsUnitConversion.h:91  (in NSFloatPixelsToAppUnits)

with the faulty line being:

100  nscoord r = aPixels * (nscoord)aAppUnitsPerPixel;

My patch just bounds-checks the multiplication product before converting it into a nscoord.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch (adding NS_COORD_IS_FLOAT case)

Added an #ifdef NS_COORD_IS_FLOAT case, so that we won't bother with the new bounds-checking after nscoords become floats.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280009 [details] [diff] [review]
patch (adding NS_COORD_IS_FLOAT case)

r=bzbarsky, but watch for performance impact!

Comment 12

[Daniel Holbert [:dholbert]]

Patch checked in to trunk. --> Fixed.

Checking in nsUnitConversion.h;
/cvsroot/mozilla/xpcom/ds/nsUnitConversion.h,v  <--  nsUnitConversion.h
new revision: 3.17; previous revision: 3.16
done

Comment 13

[Martijn Wargers (dead)]

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