r123837 Reproducibility: always Steps: N/A unfortunately as this occurs when accessing a restricted page that you cannot access. Hopefully the stacktrace is meaningful. What happened: ASSERTION FAILED: allocatedMaxLogicalWidth <= cellMaxLogicalWidth /Users/rex/WebKit/Source/WebCore/rendering/AutoTableLayout.cpp(413) : int WebCore::AutoTableLayout::calcEffectiveLogicalWidth() 1 0x108b40e92 WebCore::AutoTableLayout::calcEffectiveLogicalWidth() 2 0x108b3ef62 WebCore::AutoTableLayout::computePreferredLogicalWidths(WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) 3 0x109da30c5 WebCore::RenderTable::computePreferredLogicalWidths() 4 0x109bf53b7 WebCore::RenderBox::minPreferredLogicalWidth() const 5 0x109ba161a WebCore::RenderBlock::computeBlockPreferredLogicalWidths() 6 0x109b9f630 WebCore::RenderBlock::computePreferredLogicalWidths() 7 0x109bf53b7 WebCore::RenderBox::minPreferredLogicalWidth() const 8 0x109ba161a WebCore::RenderBlock::computeBlockPreferredLogicalWidths() 9 0x109b9f630 WebCore::RenderBlock::computePreferredLogicalWidths() 10 0x109bf53b7 WebCore::RenderBox::minPreferredLogicalWidth() const 11 0x109ba161a WebCore::RenderBlock::computeBlockPreferredLogicalWidths() 12 0x109b9f630 WebCore::RenderBlock::computePreferredLogicalWidths() 13 0x109bf53b7 WebCore::RenderBox::minPreferredLogicalWidth() const 14 0x109ba161a WebCore::RenderBlock::computeBlockPreferredLogicalWidths() 15 0x109b9f630 WebCore::RenderBlock::computePreferredLogicalWidths() 16 0x109bf53b7 WebCore::RenderBox::minPreferredLogicalWidth() const 17 0x109ba161a WebCore::RenderBlock::computeBlockPreferredLogicalWidths() 18 0x109b9f630 WebCore::RenderBlock::computePreferredLogicalWidths() 19 0x109bf53b7 WebCore::RenderBox::minPreferredLogicalWidth() const 20 0x109ba161a WebCore::RenderBlock::computeBlockPreferredLogicalWidths() 21 0x109b9f630 WebCore::RenderBlock::computePreferredLogicalWidths() 22 0x109bf53b7 WebCore::RenderBox::minPreferredLogicalWidth() const 23 0x109ba161a WebCore::RenderBlock::computeBlockPreferredLogicalWidths() 24 0x109b9f630 WebCore::RenderBlock::computePreferredLogicalWidths() 25 0x109da9e90 WebCore::RenderTableCell::computePreferredLogicalWidths() 26 0x108b3e1e4 WebCore::AutoTableLayout::recalcColumn(unsigned int) 27 0x108b3eef1 WebCore::AutoTableLayout::fullRecalc() 28 0x108b3ef56 WebCore::AutoTableLayout::computePreferredLogicalWidths(WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) 29 0x109da30c5 WebCore::RenderTable::computePreferredLogicalWidths() 30 0x109bf53b7 WebCore::RenderBox::minPreferredLogicalWidth() const 31 0x109d9f861 WebCore::RenderTable::computeLogicalWidth() Expected result: No assert failure.
This ASSERT means that our logic doesn't properly clamp a total percent above 100% and we end up allocating more than our available width. Dimitri, could it be possible for you to attach some page that reproduce the crash? It's related to an auto-table layout where all cells (or colums) have percent widths.
Found one. http://demo.pmease.com/overview/4
Hi Julien. The problem here is due to a decimal error in the float value (allocatedMaxLogicalWidth). When we split the cellMaxLogicalWidth (which is 512.00000 in this case) between different columns (based on the percentage) and add it up (in allocatedMaxLogicalWidth) we finally get 512.00006, thereby introducing an error of .00006 due to floating point rounding off in the last decimal, which in turn triggers the assert. I guess type casting to int and then checking should help in most cases. eg: ASSERT(static_cast<int>(allocatedMaxLogicalWidth) <= static_cast<int>(cellMaxLogicalWidth)); Please let me know your opinion.
I think I might've been a little off in my previous comment. Type casting to int will increase the error tolerance and is not right here. And I suppose rounding off too will not be of any help. So perhaps a better way would be to fix on an error tolerance value to accomodate the floating point rounding off errors. eg: ASSERT( (allocatedMaxLogicalWidth - cellMaxLogicalWidth) < 0.01 ); 0.01 being the highest value to accommodate floating point errors. The maximum deviation as I understand would be (no. of cols x 0.000001)
(In reply to comment #4) > I think I might've been a little off in my previous comment. Type casting to int will increase the error tolerance and is not right here. And I suppose rounding off too will not be of any help. Adding more tolerance is sweeping the dust under the carpet. If we have a rounding error, we should fix that and *not* relax our ASSERT to not trigger as this is a potential bug (here it's a pretty small amount but nothing prevents us from over-allocating more than 0.5px which will be visible). I think the main issue is that we do computation using floats for the max logical width (vs ints for min logical width). I don't see the need for the extra complexity and the potential sub-pixel precision.
Created attachment 156350 [details] Patch
Hi Julien. Have uploaded a patch that changes the float based calculations done for computing the max logical width value to int.
Comment on attachment 156350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156350&action=review The code change is fine, but this needs a test case. > Source/WebCore/ChangeLog:11 > + Existing tests should cover the changes made in this patch hence not adding any new tests. That's not true: the rounding is properly covered by the tests. However we had no test triggering the ASSERT so you need to add such a test. > Source/WebCore/rendering/AutoTableLayout.cpp:364 > + maxLogicalWidth = max(maxLogicalWidth, static_cast<int>(max(spanMaxLogicalWidth, cellMaxLogicalWidth) * 100 / cellLogicalWidth.percent())); Do we still need this static_cast<int>?
Created attachment 157868 [details] Patch
(In reply to comment #8) > (From update of attachment 156350 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156350&action=review > > The code change is fine, but this needs a test case. > > > Source/WebCore/ChangeLog:11 > > + Existing tests should cover the changes made in this patch hence not adding any new tests. > > That's not true: the rounding is properly covered by the tests. However we had no test triggering the ASSERT so you need to add such a test. > Have added a test for verifying the assert. > > Source/WebCore/rendering/AutoTableLayout.cpp:364 > > + maxLogicalWidth = max(maxLogicalWidth, static_cast<int>(max(spanMaxLogicalWidth, cellMaxLogicalWidth) * 100 / cellLogicalWidth.percent())); > > Do we still need this static_cast<int>? Yes, that is still required as the outer max() will give an error if not casted, since cellLogicalWidth.percent() is a float.
Comment on attachment 157868 [details] Patch Won’t this reintroduce bug 15010 or similar bugs?
I understand the overflow that may arise out of (int * int) operations even though the layout test case added in 15010 doesn't seem to fail with these changes. Shall make changes to take care of this scenario.
Created attachment 158324 [details] Patch
Hi Julien, Dan. Have uploaded another patch that strives to make max logical width calculations consistent with min logical width (i.e. int based) while taking care of issues raised in #15010 ( int * int overflow issue ) as well. There are two places where (int * int) operations are done in calcEffectiveLogicalWidth() which could cause overflow issues. Have thus retained the previous implementation as such for these (i.e using float) to hopefully avert any overflow issues.
Comment on attachment 158324 [details] Patch > Won’t this reintroduce bug 15010 or similar bugs? AFAICT the test added in this bug passes so at least this overflow doesn't happen. Considering our bad coverage, it doesn't mean we don't have other overflows. I believe this patch makes a good attempt at addressing the original overflow issue but without being involved in it, this is guess-work. I intent to let this patch land tomorrow as-is. Dan, feel free to override the decision.
Comment on attachment 158324 [details] Patch Clearing flags on attachment: 158324 Committed r125917: <http://trac.webkit.org/changeset/125917>
All reviewed patches have been landed. Closing bug.
Thank-you for the review Julien.