Created attachment 146968 [details] Test case With some display type combinations, blocks set to be 100% wide combined with padding or borders can cause an integer overflow and make the blocks be rounded to 0 pixels wide. Test case attached. <rdar://problem/11328762>
Created attachment 146977 [details] Patch Here's a patch, but in the case that m_*PreferredLogicalWidth is negative, it will fail. Does anyone know if either of them can be negative?
Comment on attachment 146977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review Looks good. I wish for a slightly clearer idiom for addition with pinning, maybe a helper function, but this does seem OK as is. > LayoutTests/fast/block/block-size-integer-overflow.html:14 > + window.layoutTestController.dumpAsText(); Since this is a dumpAsText test, it would be nice if the test itself explained that it was testing, how, and why, with a bit of text in the page.
Comment on attachment 146977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review > LayoutTests/fast/block/block-size-integer-overflow.html:31 > +<div style="display: -webkit-box; -webkit-box-orient: horizontal"> > + <div style="padding-left: 1px"> > + <div class="fail">FA</div> > + <div id="spacer" style="color: green; width: 100%; background-color: green"></div> > + <div class="fail">IL</div> > + </div> Why does this trigger the bug? A width of 100% should only be around 800px in this example. Is it specific to table-cell? > Source/WebCore/rendering/RenderBlock.cpp:5327 > - m_minPreferredLogicalWidth += borderAndPadding; > - m_maxPreferredLogicalWidth += borderAndPadding; > + m_minPreferredLogicalWidth = (MAX_LAYOUT_UNIT - m_minPreferredLogicalWidth) > borderAndPadding ? m_minPreferredLogicalWidth + borderAndPadding : MAX_LAYOUT_UNIT; > + m_maxPreferredLogicalWidth = (MAX_LAYOUT_UNIT - m_maxPreferredLogicalWidth) > borderAndPadding ? m_maxPreferredLogicalWidth + borderAndPadding : MAX_LAYOUT_UNIT; The same code is in RenderDeprecatedFlexibleBox.cpp and RenderFlexibleBox.cpp. Do we need to update those as well?
(In reply to comment #3) > (From update of attachment 146977 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review > > > LayoutTests/fast/block/block-size-integer-overflow.html:31 > > +<div style="display: -webkit-box; -webkit-box-orient: horizontal"> > > + <div style="padding-left: 1px"> > > + <div class="fail">FA</div> > > + <div id="spacer" style="color: green; width: 100%; background-color: green"></div> > > + <div class="fail">IL</div> > > + </div> > > Why does this trigger the bug? A width of 100% should only be around 800px in this example. Is it specific to table-cell? When width is 100%, the preferred width is MAX_LAYOUT_UNIT, which is INT_MAX. This gets scaled down to 800px later. > > > Source/WebCore/rendering/RenderBlock.cpp:5327 > > - m_minPreferredLogicalWidth += borderAndPadding; > > - m_maxPreferredLogicalWidth += borderAndPadding; > > + m_minPreferredLogicalWidth = (MAX_LAYOUT_UNIT - m_minPreferredLogicalWidth) > borderAndPadding ? m_minPreferredLogicalWidth + borderAndPadding : MAX_LAYOUT_UNIT; > > + m_maxPreferredLogicalWidth = (MAX_LAYOUT_UNIT - m_maxPreferredLogicalWidth) > borderAndPadding ? m_maxPreferredLogicalWidth + borderAndPadding : MAX_LAYOUT_UNIT; > > The same code is in RenderDeprecatedFlexibleBox.cpp and RenderFlexibleBox.cpp. Do we need to update those as well? It's possible. I should probably take a look at those too, to see if anything can come in as INT_MAX.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 146977 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review > > > > > LayoutTests/fast/block/block-size-integer-overflow.html:31 > > > +<div style="display: -webkit-box; -webkit-box-orient: horizontal"> > > > + <div style="padding-left: 1px"> > > > + <div class="fail">FA</div> > > > + <div id="spacer" style="color: green; width: 100%; background-color: green"></div> > > > + <div class="fail">IL</div> > > > + </div> > > > > Why does this trigger the bug? A width of 100% should only be around 800px in this example. Is it specific to table-cell? > > When width is 100%, the preferred width is MAX_LAYOUT_UNIT, which is INT_MAX. This gets scaled down to 800px later. > Specifically, width of 100% is MAX_LAYOUT_UNIT when used in conjunction with AutoTableLayout. In other cases, it's not. This bug is only likely to come up when AutoTableLayout is in use. I'm not 100% sure what uses AutoTableLayout, though. It's possible that this could come up if people specify absurdly large (e.g. close to INT_MAX) block widths. Actually, looking back at this, the line that sets it to MAX_LAYOUT_UNIT is prefixed by this comment: // if there was no remaining percent, maxWidth is invalid So it makes me wonder if we're doing the right thing here anyway. (This is in AutoTableLayout::computePreferredLogicalWidths)
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 146977 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review > > > > > > > LayoutTests/fast/block/block-size-integer-overflow.html:31 > > > > +<div style="display: -webkit-box; -webkit-box-orient: horizontal"> > > > > + <div style="padding-left: 1px"> > > > > + <div class="fail">FA</div> > > > > + <div id="spacer" style="color: green; width: 100%; background-color: green"></div> > > > > + <div class="fail">IL</div> > > > > + </div> > > > > > > Why does this trigger the bug? A width of 100% should only be around 800px in this example. Is it specific to table-cell? > > > > When width is 100%, the preferred width is MAX_LAYOUT_UNIT, which is INT_MAX. This gets scaled down to 800px later. > > > > Specifically, width of 100% is MAX_LAYOUT_UNIT when used in conjunction with AutoTableLayout. In other cases, it's not. This bug is only likely to come up when AutoTableLayout is in use. I'm not 100% sure what uses AutoTableLayout, though. It's possible that this could come up if people specify absurdly large (e.g. close to INT_MAX) block widths. Ah, thanks, I see the code in AutoTableLayout.cpp now. It looks like only max preferred width is set to MAX_LAYOUT_UNIT. Should we remove the check on min? I'll see if I can trigger an overflow in some of the flexbox cases. > Actually, looking back at this, the line that sets it to MAX_LAYOUT_UNIT is prefixed by this comment: > // if there was no remaining percent, maxWidth is invalid > So it makes me wonder if we're doing the right thing here anyway. (This is in AutoTableLayout::computePreferredLogicalWidths) Adding Julien who may know something about this class.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > (From update of attachment 146977 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review > > > > > > > > > LayoutTests/fast/block/block-size-integer-overflow.html:31 > > > > > +<div style="display: -webkit-box; -webkit-box-orient: horizontal"> > > > > > + <div style="padding-left: 1px"> > > > > > + <div class="fail">FA</div> > > > > > + <div id="spacer" style="color: green; width: 100%; background-color: green"></div> > > > > > + <div class="fail">IL</div> > > > > > + </div> > > > > > > > > Why does this trigger the bug? A width of 100% should only be around 800px in this example. Is it specific to table-cell? > > > > > > When width is 100%, the preferred width is MAX_LAYOUT_UNIT, which is INT_MAX. This gets scaled down to 800px later. > > > > > > > Specifically, width of 100% is MAX_LAYOUT_UNIT when used in conjunction with AutoTableLayout. In other cases, it's not. This bug is only likely to come up when AutoTableLayout is in use. I'm not 100% sure what uses AutoTableLayout, though. It's possible that this could come up if people specify absurdly large (e.g. close to INT_MAX) block widths. > > Ah, thanks, I see the code in AutoTableLayout.cpp now. It looks like only max preferred width is set to MAX_LAYOUT_UNIT. Should we remove the check on min? > > I'll see if I can trigger an overflow in some of the flexbox cases. After looking over and stepping through the code, I think only RenderBox needs to be changed, and the check on min can be removed. It looks like this only happens when RenderBox::sizesToIntrinsicLogicalWidth returns true during RenderBlock::computeLogicalWidth occurs. This causes the intrinsic width to get calculated and then overflown when padding are added on. I am seeing other places that might overflow, though.
Created attachment 147180 [details] overflow from margin Here's another test case which overflows RenderBlock.cpp:5761 (w = childMaxPreferredLogicalWidth + margin;). I suspect any time we try to add to m_maxPreferredLogicalWidth, we can possibly overflow. One option would be to add a helper function for adding to maxPreferredLogicalWidth and try to update all callers to use it. We could also try to make LayoutUnit smart and have it detect when it's going to overflow. That might have a runtime cost since it would impact all adds. Or maybe the code in AutoTableLayout is fishy? Maybe there's some way to produce the same layout without using MAX_LAYOUT_UNIT? If a cell has a percentage width, can we calculate the size based on the table width?
Created attachment 147182 [details] overflow in deprecated flexbox Here's an example of overflowing RenderDeprecatedFlexibleBox.cpp:225. Changing display: -webkit-box to display: -webkit-flex would overflow RenderFlexibleBox.cpp.
I think this is a regression from https://bugs.webkit.org/show_bug.cgi?id=56052 . See comment #7 where Hyatt asks if there's a layout test for this logic change. Looking in AutoTableLayout.cpp, there are other cases where we clamp the value of maxWidth to MAX_LAYOUT_UNIT / 2.0 in the same function. I think we should do that here. This doesn't appear to break any of the table layout tests for me.
(In reply to comment #10) > Looking in AutoTableLayout.cpp, there are other cases where we clamp the value of maxWidth to MAX_LAYOUT_UNIT / 2.0 in the same function. I think we should do that here. This doesn't appear to break any of the table layout tests for me. I'm a bit suspicious of clamping it to MAX_LAYOUT_UNIT / 2.0f. It seems a bit like a hack, and the fact that we're using f means that we lose some bits to the mantissa (or, depending on how it rounds, we only lose one bit, but the value increases). And although I highly doubt that anyone will set any of the dimensions to anywhere near MAX_LAYOUT_UNIT (within several orders of magnitude), it still makes me wonder what will happen if some of that addition can overflow. It will prevent the trivial cases, which are demonstrated in these repros, from breaking, though.
(In reply to comment #8) > Or maybe the code in AutoTableLayout is fishy? Maybe there's some way to produce the same layout without using MAX_LAYOUT_UNIT? If a cell has a percentage width, can we calculate the size based on the table width? I looked into this, and that's how width is generally calculated, except that the codepath that's gone down asks the table cell how wide it WANTS to be, if it doesn't care about how wide anything is. As such, the available size isn't passed down, and we can't find it because the operation is supposed to be independent of it.
> Maybe there's some way to produce the same layout without using MAX_LAYOUT_UNIT? I think Tony's proposal would work. We don't care about the exact value, it just has to be arbitrary large. The code already uses a cap of MAX_LAYOUT_UNIT / 2.0f as a magic constant for the percent case so it would just be an extension of that. As a side note, several other places are using the magic constant of 15,000 to represent the max width (BLOCK_MAX_WIDTH, TABLE_MAX_WIDTH (for fixed table layout) and cLargeLogicalWidth for RenderMathMLBlock). I would support going with this value as it makes the 2 table layout more consistent and alleviate the risk of overflow by a bigger margin.
(In reply to comment #13) > > Maybe there's some way to produce the same layout without using MAX_LAYOUT_UNIT? > > I think Tony's proposal would work. We don't care about the exact value, it just has to be arbitrary large. The code already uses a cap of MAX_LAYOUT_UNIT / 2.0f as a magic constant for the percent case so it would just be an extension of that. > > As a side note, several other places are using the magic constant of 15,000 to represent the max width (BLOCK_MAX_WIDTH, TABLE_MAX_WIDTH (for fixed table layout) and cLargeLogicalWidth for RenderMathMLBlock). I would support going with this value as it makes the 2 table layout more consistent and alleviate the risk of overflow by a bigger margin. I think the smaller magic constant is the way to go. 15,000 seems good enough. Another case where MAX/2 wouldn't work is calculating the preferred width of a horizontal flexbox containing 100% width table cells. The preferred width of a horizontal flexbox is the sum of it's flex-items' preferred widths. So, if you had 3 items, you'd overflow again. With 15,000, you'd need a ridiculous number of items before we hit this case. In short, good enough.
Created attachment 147398 [details] Patch
Comment on attachment 147398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147398&action=review > Source/WebCore/rendering/AutoTableLayout.cpp:224 > +// Use a very large value (in effect infinite). But not too large! > +// Keep this in synch with BLOCK_MAX_WIDTH in RenderBlock.cpp and TABLE_MAX_WIDTH in FixedTableLayout.cpp > +#define TABLE_MAX_WIDTH 15000 Can you remove BLOCK_MAX_WIDTH from RenderBlock.cpp? It doesn't look like it's used anymore. Then you can update the comment here and in FixedTableLayout.cpp. We might just want to somehow share the value between FixedTableLayout.cpp and AutoTableLayout.cpp (maybe declare a static in TableLayout.h and instantiate it in RenderTable.cpp?), but that seems less important.
Comment on attachment 147398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147398&action=review >> Source/WebCore/rendering/AutoTableLayout.cpp:224 >> +#define TABLE_MAX_WIDTH 15000 > > Can you remove BLOCK_MAX_WIDTH from RenderBlock.cpp? It doesn't look like it's used anymore. Then you can update the comment here and in FixedTableLayout.cpp. We might just want to somehow share the value between FixedTableLayout.cpp and AutoTableLayout.cpp (maybe declare a static in TableLayout.h and instantiate it in RenderTable.cpp?), but that seems less important. I would argue that moving the #define to TableLayout.h is the way to go. That would avoid any duplication and unneeded sync'ing between the 2 values. Also this could be turned into a proper constant (this is really a nit).
Created attachment 147414 [details] Patch
Committed r120257: <http://trac.webkit.org/changeset/120257>