Bug 92471 – ASSERTION FAILED: allocatedMaxLogicalWidth <= cellMaxLogicalWidth : int WebCore::AutoTableLayout::calcEffectiveLogicalWidth()

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Dimitris Apostolou]

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.

[Julien Chaffraix]

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.

[Dimitris Apostolou]

Found one.

http://demo.pmease.com/overview/4

[Arpita Bahuguna]

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.

[Arpita Bahuguna]

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)

[Julien Chaffraix]

(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.

[Arpita Bahuguna]

Created attachment 156350 [details]
Patch

[Arpita Bahuguna]

Hi Julien. Have uploaded a patch that changes the float based calculations done for computing the max logical width value to int.

[Julien Chaffraix]

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>?

[Arpita Bahuguna]

Created attachment 157868 [details]
Patch

[Arpita Bahuguna]

(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?

[Arpita Bahuguna]

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.

[Arpita Bahuguna]

Created attachment 158324 [details]
Patch

[Arpita Bahuguna]

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.

[Julien Chaffraix]

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.

[WebKit Review Bot]

Comment on attachment 158324 [details]
Patch

Clearing flags on attachment: 158324

Committed r125917: <http://trac.webkit.org/changeset/125917>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arpita Bahuguna]

Thank-you for the review Julien.