Bug 224310 – Division by zero in WebCore::RenderGrid::computeAutoRepeatTracksCount (RenderGrid.cpp:525)

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

ASSERTION FAILED: !availableSize || *availableSize >= 0
./rendering/RenderGrid.cpp(316) : WebCore::LayoutUnit WebCore::RenderGrid::gridGap(WebCore::GridTrackSizingDirection, Optional<WebCore::LayoutUnit>) const
1   0x5c6b33bf9 WTFCrash
2   0x5a90534cb WTFCrashWithInfo(int, char const*, char const*, int)
3   0x5ad61d0e2 WebCore::RenderGrid::gridGap(WebCore::GridTrackSizingDirection, WTF::Optional<WebCore::LayoutUnit>) const
4   0x5ad61e66f WebCore::RenderGrid::computeAutoRepeatTracksCount(WebCore::GridTrackSizingDirection, WTF::Optional<WebCore::LayoutUnit>) const
5   0x5ad61c36f WebCore::RenderGrid::placeItemsOnGrid(WebCore::GridTrackSizingAlgorithm&, WTF::Optional<WebCore::LayoutUnit>) const
6   0x5ad61bc9c WebCore::RenderGrid::layoutBlock(bool, WebCore::LayoutUnit)
7   0x5ad6a1c42 WebCore::RenderLayerScrollableArea::updateScrollbarsAfterLayout()
8   0x5ad65938c WebCore::RenderLayerScrollableArea::updateScrollInfoAfterLayout()
9   0x5ad6591b5 WebCore::RenderLayer::updateScrollInfoAfterLayout()
10  0x5ad506db2 WebCore::RenderBlock::endAndCommitUpdateScrollInfoAfterLayoutTransaction()
11  0x5ad61c0df WebCore::RenderGrid::layoutBlock(bool, WebCore::LayoutUnit)
12  0x5ad507179 WebCore::RenderBlock::layout()
13  0x5ad522182 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
14  0x5ad520b14 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
15  0x5ad51f993 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
16  0x5ad507179 WebCore::RenderBlock::layout()
17  0x5ad7a6313 WebCore::RenderView::layout()
18  0x5acc89901 WebCore::FrameViewLayoutContext::layout()
19  0x5abeb2836 WebCore::Document::implicitClose()
20  0x5aca8159b WebCore::FrameLoader::checkCallImplicitClose()
21  0x5aca80fca WebCore::FrameLoader::checkCompleted()
22  0x5aca7f0b7 WebCore::FrameLoader::finishedParsing()
23  0x5abec7476 WebCore::Document::finishedParsing()
24  0x5ac6010d8 WebCore::HTMLConstructionSite::finishedParsing()
25  0x5ac646f57 WebCore::HTMLTreeBuilder::finished()
26  0x5ac608708 WebCore::HTMLDocumentParser::end()
27  0x5ac6063d8 WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
28  0x5ac606117 WebCore::HTMLDocumentParser::prepareToStopParsing()
29  0x5ac608772 WebCore::HTMLDocumentParser::attemptToEnd()
30  0x5ac608839 WebCore::HTMLDocumentParser::finish()
31  0x5aca58e44 WebCore::DocumentWriter::end()
#CRASHED - com.apple.WebKit.WebContent.Development (pid 2680)

<rdar://76344587>

[Ryosuke Niwa]

Debug assertion:
ASSERTION FAILED: !availableSize || *availableSize >= 0
./rendering/RenderGrid.cpp(316) : WebCore::LayoutUnit WebCore::RenderGrid::gridGap(WebCore::GridTrackSizingDirection, Optional<WebCore::LayoutUnit>) const
1   0x5c6b33bf9 WTFCrash
2   0x5a90534cb WTFCrashWithInfo(int, char const*, char const*, int)
3   0x5ad61d0e2 WebCore::RenderGrid::gridGap(WebCore::GridTrackSizingDirection, WTF::Optional<WebCore::LayoutUnit>) const
4   0x5ad61e66f WebCore::RenderGrid::computeAutoRepeatTracksCount(WebCore::GridTrackSizingDirection, WTF::Optional<WebCore::LayoutUnit>) const
5   0x5ad61c36f WebCore::RenderGrid::placeItemsOnGrid(WebCore::GridTrackSizingAlgorithm&, WTF::Optional<WebCore::LayoutUnit>) const
6   0x5ad61bc9c WebCore::RenderGrid::layoutBlock(bool, WebCore::LayoutUnit)
7   0x5ad6a1c42 WebCore::RenderLayerScrollableArea::updateScrollbarsAfterLayout()
8   0x5ad65938c WebCore::RenderLayerScrollableArea::updateScrollInfoAfterLayout()
9   0x5ad6591b5 WebCore::RenderLayer::updateScrollInfoAfterLayout()
10  0x5ad506db2 WebCore::RenderBlock::endAndCommitUpdateScrollInfoAfterLayoutTransaction()
11  0x5ad61c0df WebCore::RenderGrid::layoutBlock(bool, WebCore::LayoutUnit)
12  0x5ad507179 WebCore::RenderBlock::layout()
13  0x5ad522182 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
14  0x5ad520b14 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
15  0x5ad51f993 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
16  0x5ad507179 WebCore::RenderBlock::layout()
17  0x5ad7a6313 WebCore::RenderView::layout()
18  0x5acc89901 WebCore::FrameViewLayoutContext::layout()
19  0x5abeb2836 WebCore::Document::implicitClose()
20  0x5aca8159b WebCore::FrameLoader::checkCallImplicitClose()
21  0x5aca80fca WebCore::FrameLoader::checkCompleted()
22  0x5aca7f0b7 WebCore::FrameLoader::finishedParsing()
23  0x5abec7476 WebCore::Document::finishedParsing()
24  0x5ac6010d8 WebCore::HTMLConstructionSite::finishedParsing()
25  0x5ac646f57 WebCore::HTMLTreeBuilder::finished()
26  0x5ac608708 WebCore::HTMLDocumentParser::end()
27  0x5ac6063d8 WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
28  0x5ac606117 WebCore::HTMLDocumentParser::prepareToStopParsing()
29  0x5ac608772 WebCore::HTMLDocumentParser::attemptToEnd()
30  0x5ac608839 WebCore::HTMLDocumentParser::finish()
31  0x5aca58e44 WebCore::DocumentWriter::end()
#CRASHED - com.apple.WebKit.WebContent.Development (pid 2680)

[Ryosuke Niwa]

I can reproduce this with WebKitTestRunner at r275541.

[Ryosuke Niwa]

Created attachment 425448 [details]
Test

[Rob Buis]

Created attachment 425484 [details]
Patch

[Ryosuke Niwa]

There is no security implication here, right?

[Rob Buis]

(In reply to Ryosuke Niwa from comment #5)
> There is no security implication here, right?

I do not think so, correct.

[Manuel Rego Casasnovas]

Comment on attachment 425484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425484&action=review

The fix LGTM, I'm surprised we don't have clampNegativeToZero() for LayoutUnits in WebKit, but essentially is the same that you're doing there.

> Source/WebCore/rendering/RenderBox.h:324
> +    LayoutUnit overridingContentLogicalHeight() const { return std::max(LayoutUnit(), overridingLogicalHeight() - borderAndPaddingLogicalHeight() - scrollbarLogicalHeight()); }

I'm wondering why were not clamping this before, I believe Sergio introduced this code recently. It'd be nice to know his thoughts on this.

> LayoutTests/fast/css-grid-layout/negative-overriding-content-logical-height-crash.html:1
> +<style>

This test is very weird, can we get a reduced version of it, not sure if we need such a bunch of nested inline-grids.

Also this is using quirks mode, is that needed?

Another question, can we just put the test under WPT?

> LayoutTests/fast/css-grid-layout/negative-overriding-content-logical-height-crash.html:4
> +    overflow-y: hidden;

Does this affect the test?

> LayoutTests/fast/css-grid-layout/negative-overriding-content-logical-height-crash.html:7
> +  style, body, head, div {

"head" here doesn't look needed, but dunno.

"style" here is kind of weird too, but maybe it's needed.

[Sergio Villar Senin]

Comment on attachment 425484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425484&action=review

>> Source/WebCore/rendering/RenderBox.h:324
>> +    LayoutUnit overridingContentLogicalHeight() const { return std::max(LayoutUnit(), overridingLogicalHeight() - borderAndPaddingLogicalHeight() - scrollbarLogicalHeight()); }
> 
> I'm wondering why were not clamping this before, I believe Sergio introduced this code recently. It'd be nice to know his thoughts on this.

Well I am basically not sure who should do the clamping. Negative values make sense in flexbox for example but only as intermediate results of the algorithm, not in the final results.

That said I guess the question here is whether negative overriding sizes make sense. This would define who should do the clamping, either here or in the caller. I guess they don't, so this could be correct.

>> LayoutTests/fast/css-grid-layout/negative-overriding-content-logical-height-crash.html:4
>> +    overflow-y: hidden;
> 
> Does this affect the test?

I would bet that the first to 0's are not needed, it might be enough with having a 1px column before the auto repeat.

Also agree with Rego about trying to reduce it a bit more.

[Rob Buis]

Created attachment 425607 [details]
Patch

[Rob Buis]

Comment on attachment 425484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425484&action=review

>>> LayoutTests/fast/css-grid-layout/negative-overriding-content-logical-height-crash.html:4
>>> +    overflow-y: hidden;
>> 
>> Does this affect the test?
> 
> I would bet that the first to 0's are not needed, it might be enough with having a 1px column before the auto repeat.
> 
> Also agree with Rego about trying to reduce it a bit more.

Rego and myself found out for the division by zero problem to reproduce, we do need Ryosuke's original test case, so I reverted back to that one.

[EWS]

Committed r275823 (236393@main): <https://commits.webkit.org/236393@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425607 [details].