Bug 92471 - ASSERTION FAILED: allocatedMaxLogicalWidth <= cellMaxLogicalWidth : int WebCore::AutoTableLayout::calcEffectiveLogicalWidth()
Summary: ASSERTION FAILED: allocatedMaxLogicalWidth <= cellMaxLogicalWidth : int WebCo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel Other
: P2 Normal
Assignee: Nobody
URL: http://demo.pmease.com/overview/4
Keywords: NeedsReduction
Depends on:
Blocks:
 
Reported: 2012-07-27 01:15 PDT by Dimitris Apostolou
Modified: 2012-08-17 11:41 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.75 KB, patch)
2012-08-03 06:35 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (10.43 KB, patch)
2012-08-11 05:21 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (8.27 KB, patch)
2012-08-14 07:35 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitris Apostolou 2012-07-27 01:15:32 PDT
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.
Comment 1 Julien Chaffraix 2012-07-27 16:08:25 PDT
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.
Comment 2 Dimitris Apostolou 2012-07-27 16:40:20 PDT
Found one.

http://demo.pmease.com/overview/4
Comment 3 Arpita Bahuguna 2012-07-28 05:16:05 PDT
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.
Comment 4 Arpita Bahuguna 2012-07-28 07:56:06 PDT
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)
Comment 5 Julien Chaffraix 2012-07-30 11:13:10 PDT
(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.
Comment 6 Arpita Bahuguna 2012-08-03 06:35:27 PDT
Created attachment 156350 [details]
Patch
Comment 7 Arpita Bahuguna 2012-08-04 02:01:03 PDT
Hi Julien. Have uploaded a patch that changes the float based calculations done for computing the max logical width value to int.
Comment 8 Julien Chaffraix 2012-08-06 10:16:25 PDT
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>?
Comment 9 Arpita Bahuguna 2012-08-11 05:21:52 PDT
Created attachment 157868 [details]
Patch
Comment 10 Arpita Bahuguna 2012-08-11 05:39:29 PDT
(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 11 mitz 2012-08-11 09:30:52 PDT
Comment on attachment 157868 [details]
Patch

Won’t this reintroduce bug 15010 or similar bugs?
Comment 12 Arpita Bahuguna 2012-08-13 07:56:59 PDT
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.
Comment 13 Arpita Bahuguna 2012-08-14 07:35:27 PDT
Created attachment 158324 [details]
Patch
Comment 14 Arpita Bahuguna 2012-08-14 07:43:13 PDT
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 15 Julien Chaffraix 2012-08-15 11:29:20 PDT
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 16 WebKit Review Bot 2012-08-17 11:32:12 PDT
Comment on attachment 158324 [details]
Patch

Clearing flags on attachment: 158324

Committed r125917: <http://trac.webkit.org/changeset/125917>
Comment 17 WebKit Review Bot 2012-08-17 11:32:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Arpita Bahuguna 2012-08-17 11:41:37 PDT
Thank-you for the review Julien.