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).
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!
> 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.
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.
*** Bug 85651 has been marked as a duplicate of this bug. ***
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?
(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.
Created attachment 176651 [details] Patch
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.
Created attachment 176655 [details] Patch
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.
Created attachment 176872 [details] Patch
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.
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).
Created attachment 180843 [details] Patch
(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.
Created attachment 180844 [details] Patch
Created attachment 180845 [details] Patch
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.
(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.
> 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.
(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.
Created attachment 185886 [details] Patch
Created attachment 185893 [details] Patch
Created attachment 185898 [details] Patch
The EWS bots don't like that I removed images that lack the proper mime-type. Please ignore the purple.
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.
This was fixed in r143669.