RESOLVED FIXED 68744
Column width calculation wraps an unsigned int 
https://bugs.webkit.org/show_bug.cgi?id=68744
Summary Column width calculation wraps an unsigned int
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
DetailsFormatted DiffDiff
Patch (1.76 KB, patch)
2012-11-28 23:08 PST, Qiankun Miao 		no flags
DetailsFormatted DiffDiff
Patch (2.06 KB, patch)
2012-11-29 18:02 PST, Qiankun Miao 		no flags
DetailsFormatted DiffDiff
Patch (3.01 KB, patch)
2012-12-27 21:28 PST, Qiankun Miao 		no flags
DetailsFormatted DiffDiff
Patch (2.77 KB, patch)
2012-12-27 21:38 PST, Qiankun Miao 		no flags
DetailsFormatted DiffDiff
Patch (2.77 KB, patch)
2012-12-27 21:40 PST, Qiankun Miao 		no flags
DetailsFormatted DiffDiff
Patch (5.55 KB, patch)
2013-01-31 16:05 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (5.55 KB, patch)
2013-01-31 16:38 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (6.05 KB, patch)
2013-01-31 16:49 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
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.