Bug 68744

Summary:

Column width calculation wraps an unsigned int

Product:

WebKit

Reporter:

Levi Weintraub <leviw>

Component:

Layout and Rendering

Assignee:

Levi Weintraub <leviw>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aestes, bdakin, eae, eric, hyatt, jamesr, jchaffraix, koivisto, leviw, ojan.autocc, ojan, pkasting, qiankun.miao, simon.fraser, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

60318

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Levi Weintraub

Reported 2011-09-23 16:03:55 PDT

Load fast/block/float/float-not-removed-from-next-sibling4.html Notice the horizontal scrollbar. If you're no a debug build, you'll also notice scrolling all the way to the right results in the debug red fill. To see what's happening, look at the following line in RenderBlock::calcColumnWidth (RenderBlock's too big for a trac link): desiredColumnWidth = max<int>(0, (availWidth - ((desiredColumnCount - 1) * colGap)) / desiredColumnCount); desiredColumnCount is unsigned, and the compiler implicitly converts the other parameters to unsigned despite being ints. The end result is a massive number. Explicitly making desiredColumnCount an int fixes this issue, but breaks the test (the image no longer paints because the column width is incorrectly zero).

Attachments
Patch (1.78 KB, patch) 2012-11-28 22:47 PST, Qiankun Miao	no flags	Details Formatted Diff Diff
Patch (1.76 KB, patch) 2012-11-28 23:08 PST, Qiankun Miao	no flags	Details Formatted Diff Diff
Patch (2.06 KB, patch) 2012-11-29 18:02 PST, Qiankun Miao	no flags	Details Formatted Diff Diff
Patch (3.01 KB, patch) 2012-12-27 21:28 PST, Qiankun Miao	no flags	Details Formatted Diff Diff
Patch (2.77 KB, patch) 2012-12-27 21:38 PST, Qiankun Miao	no flags	Details Formatted Diff Diff
Patch (2.77 KB, patch) 2012-12-27 21:40 PST, Qiankun Miao	no flags	Details Formatted Diff Diff
Patch (5.55 KB, patch) 2013-01-31 16:05 PST, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch (5.55 KB, patch) 2013-01-31 16:38 PST, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch (6.05 KB, patch) 2013-01-31 16:49 PST, Emil A Eklund	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Levi Weintraub

Comment 1 2011-10-13 15:30:28 PDT

Just found another spot in Column layout where this occurs. RenderBlock::adjustRectForColumns overflows the unsigned "endColumn" variable when laying out the <p> child in span-as-immediate-child-complex-splitting.html. I'm a little confused why we need unsigned values here, but it seems bad to be relying on wrapping!

Levi Weintraub

Comment 2 2011-10-13 15:42:29 PDT

> I'm a little confused why we need unsigned values here, but it seems bad to be relying on wrapping! Alright, I understand in this 2nd case why we may want unsigned (these are indexes), but it still doesn't make sense to me to pass an index like 4 billion to columnRectAt when we actually only have 2 columns.

Levi Weintraub

Comment 3 2012-03-12 16:22:51 PDT

We've added assertions to FractionalLayoutUnit that catches this sort of behavior, and it would be a shame to have to disable this potential protection because we find ourselves in this case so frequently. I'd really appreciate input from someone more familiar with this algorithm.

Emil A Eklund

Comment 4 2012-05-04 12:40:37 PDT

*** Bug 85651 has been marked as a duplicate of this bug. ***

Peter Kasting

Comment 5 2012-07-31 17:30:35 PDT

Levi/Emil, the current Chromium test results here have no scrollbars (the expected images have scrollbars). Does that mean this bug is fixed and the test can be rebaselined?

Levi Weintraub

Comment 6 2012-07-31 17:33:33 PDT

(In reply to comment #5) > Levi/Emil, the current Chromium test results here have no scrollbars (the expected images have scrollbars). Does that mean this bug is fixed and the test can be rebaselined? Not unless there are two boxes in the current expectations. The scrollbars are an artifact of this wrapping. It made the element, and hence the page extremely wide. While they shouldn't be in the page, there should be a black box and a grey one.

Qiankun Miao

Comment 7 2012-11-28 22:47:20 PST

Created attachment 176651 [details] Patch

WebKit Review Bot

Comment 8 2012-11-28 22:49:53 PST

Attachment 176651 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Qiankun Miao

Comment 9 2012-11-28 23:08:31 PST

Created attachment 176655 [details] Patch

Levi Weintraub

Comment 10 2012-11-29 10:58:47 PST

Comment on attachment 176655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176655&action=review > Source/WebCore/ChangeLog:9 > + When running fast/multicol/span/span-as-immediate-child-complex-splitting.html > + with debug version, there is an overflow. This patch fixes the overflow. The overflow occurs in Release builds as well. You should comment on the actual fix here instead of just saying it's fixed. > Source/WebCore/rendering/RenderBlock.cpp:5489 > + // endOffset should be always larger than startOffset. This comment is probably superfluous.

Qiankun Miao

Comment 11 2012-11-29 18:02:42 PST

Created attachment 176872 [details] Patch

Qiankun Miao

Comment 12 2012-11-29 18:05:16 PST

Thanks for review. Refine the comments. Please help to review. (In reply to comment #10) > (From update of attachment 176655 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176655&action=review > > Source/WebCore/ChangeLog:9 > > + When running fast/multicol/span/span-as-immediate-child-complex-splitting.html > > + with debug version, there is an overflow. This patch fixes the overflow. > The overflow occurs in Release builds as well. You should comment on the actual fix here instead of just saying it's fixed. > > Source/WebCore/rendering/RenderBlock.cpp:5489 > > + // endOffset should be always larger than startOffset. > This comment is probably superfluous.

Levi Weintraub

Comment 13 2012-12-27 11:35:45 PST

Comment on attachment 176872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176872&action=review This affects some test cases, right? I'd expect to see updated test expectations or a test that covers this along with the code change. > Source/WebCore/ChangeLog:6 > + Reviewed by Levi Weintraub. You're not supposed to fill this in until someone gives you an R+ ("Review Granted"). The tools will automatically fill this in if you use them to land (like "webkit-patch land-safely" or setting the cq+ bit on the patch).

Qiankun Miao

Comment 14 2012-12-27 21:28:10 PST

Created attachment 180843 [details] Patch

Qiankun Miao

Comment 15 2012-12-27 21:34:17 PST

(In reply to comment #13) > (From update of attachment 176872 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176872&action=review > > This affects some test cases, right? I'd expect to see updated test expectations or a test that covers this along with the code change. Yes, some test cases were effect. Add a ASSERT to crash these test cases, if startOffset is larger than endOffset. Is it enough for the test cases? > > > Source/WebCore/ChangeLog:6 > > + Reviewed by Levi Weintraub. > > You're not supposed to fill this in until someone gives you an R+ ("Review Granted"). The tools will automatically fill this in if you use them to land (like "webkit-patch land-safely" or setting the cq+ bit on the patch). Thanks for reminder.

Qiankun Miao

Comment 16 2012-12-27 21:38:09 PST

Created attachment 180844 [details] Patch

Qiankun Miao

Comment 17 2012-12-27 21:40:59 PST

Created attachment 180845 [details] Patch

Levi Weintraub

Comment 18 2012-12-27 22:04:33 PST

Comment on attachment 180845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180845&action=review > Source/WebCore/ChangeLog:24 > + Tests: fast/multicol/span/span-as-nested-columns-child-dynamic.html > + fast/multicol/span/span-as-immediate-columns-child-dynamic.html > + fast/multicol/span/span-as-immediate-columns-child.html > + fast/multicol/span/span-as-nested-columns-child.html > + fast/multicol/span/span-as-immediate-child-generated-content.html > + fast/multicol/span/span-as-immediate-columns-child-removal.html > + fast/multicol/span/generated-child-split-flow-crash.html > + fast/multicol/span/span-as-immediate-child-property-removal.html > + fast/multicol/span/span-as-immediate-child-complex-splitting.html You list a bunch of tests, but there are no new test expectations nor new tests added in this patch. You should either have a new test or new expectations for an existing test. > Source/WebCore/rendering/RenderBlock.cpp:5490 > + ASSERT(startOffset <= endOffset); This assert doesn't really add anything.

Qiankun Miao

Comment 19 2012-12-27 23:27:57 PST

(In reply to comment #18) > (From update of attachment 180845 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180845&action=review > > > Source/WebCore/ChangeLog:24 > > + Tests: fast/multicol/span/span-as-nested-columns-child-dynamic.html > > + fast/multicol/span/span-as-immediate-columns-child-dynamic.html > > + fast/multicol/span/span-as-immediate-columns-child.html > > + fast/multicol/span/span-as-nested-columns-child.html > > + fast/multicol/span/span-as-immediate-child-generated-content.html > > + fast/multicol/span/span-as-immediate-columns-child-removal.html > > + fast/multicol/span/generated-child-split-flow-crash.html > > + fast/multicol/span/span-as-immediate-child-property-removal.html > > + fast/multicol/span/span-as-immediate-child-complex-splitting.html > > You list a bunch of tests, but there are no new test expectations nor new tests added in this patch. You should either have a new test or new expectations for an existing test. These test cases won't raise overflow error to stderr. But this patch doesn't effect the rendering result. So there is no change to the expect file. This because the overflow will only enlarge the repaint rectangle while it doesn't effect the paint result. Do you have some comments on how to add test cases? > > > Source/WebCore/rendering/RenderBlock.cpp:5490 > > + ASSERT(startOffset <= endOffset); > > This assert doesn't really add anything.

Levi Weintraub

Comment 20 2012-12-27 23:33:56 PST

> These test cases won't raise overflow error to stderr. But this patch doesn't effect the rendering result. So there is no change to the expect file. This because the overflow will only enlarge the repaint rectangle while it doesn't effect the paint result. Do you have some comments on how to add test cases? If it enlarges the repaint rectangle, use a repaint test.

Qiankun Miao

Comment 21 2012-12-28 00:53:56 PST

(In reply to comment #20) > > These test cases won't raise overflow error to stderr. But this patch doesn't effect the rendering result. So there is no change to the expect file. This because the overflow will only enlarge the repaint rectangle while it doesn't effect the paint result. Do you have some comments on how to add test cases? > > If it enlarges the repaint rectangle, use a repaint test. It's only an intermediate rectangle. Finally the intersection of the intermediate rectangle and visibleContentRect will be calculated. This step will elimate the side effect the overflow.

Emil A Eklund

Comment 22 2013-01-31 16:05:12 PST

Created attachment 185886 [details] Patch

Emil A Eklund

Comment 23 2013-01-31 16:38:05 PST

Created attachment 185893 [details] Patch

Emil A Eklund

Comment 24 2013-01-31 16:49:26 PST

Created attachment 185898 [details] Patch

Emil A Eklund

Comment 25 2013-01-31 16:54:55 PST

The EWS bots don't like that I removed images that lack the proper mime-type. Please ignore the purple.

Levi Weintraub

Comment 26 2013-01-31 16:58:22 PST

Comment on attachment 185898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185898&action=review > Source/WebCore/ChangeLog:11 > + Change RenderBlock::adjustRectForColumns to not overflow when > + there is insufficient space. The spec is unclear on how this > + situation should be handled but overflowing is certainly not > + the right thing to do. Any chance we could ping the spec writers on what they'd like done here? Our current behavior is clearly wrong, and I'm fine with this as a solution, but it'd be best if it were defined. What does FFX do? > LayoutTests/ChangeLog:8 > + Update expected results for fast/block/float/float-not-removed-from-next-sibling4.html Explaining what the new result is would be good.

Emil A Eklund

Comment 27 2013-03-06 12:11:15 PST

This was fixed in r143669.

Note You need to log in before you can comment on or make changes to this bug.