Bug 221439

Summary:

Division by zero in computeAutoRepeatTracksCount

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

Layout and Rendering

Assignee:

Rob Buis <rbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cgarcia, darin, ews-feeder, fred.wang, gpoo, jfernandez, koivisto, product-security, rbuis, rego, simon.fraser, svillar, webkit-bot-watchers-bugzilla, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=222421
https://bugs.webkit.org/show_bug.cgi?id=224310

Bug Depends on:

191627

Bug Blocks:

Attachments:

Description	Flags
Test	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none

Ryosuke Niwa

Reported 2021-02-04 17:53:30 PST

Created attachment 419346 [details] Test e.g. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00007fff3b5d698f WebCore::RenderGrid::computeAutoRepeatTracksCount(WebCore::GridTrackSizingDirection, WTF::Optional<WebCore::LayoutUnit>) const + 1983 1 com.apple.WebCore 0x00007fff3b5d4ab7 WebCore::RenderGrid::placeItemsOnGrid(WebCore::GridTrackSizingAlgorithm&, WTF::Optional<WebCore::LayoutUnit>) const + 119 2 com.apple.WebCore 0x00007fff3b5d31e8 WebCore::RenderGrid::layoutBlock(bool, WebCore::LayoutUnit) + 1464 3 com.apple.WebCore 0x00007fff398bb45a WebCore::RenderBlock::layout() + 42 4 com.apple.WebCore 0x00007fff3b5d40f3 WebCore::RenderGrid::layoutBlock(bool, WebCore::LayoutUnit) + 5315 5 com.apple.WebCore 0x00007fff398bb45a WebCore::RenderBlock::layout() + 42 6 com.apple.WebCore 0x00007fff3b5610a6 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 3926 7 com.apple.WebCore 0x00007fff3b55ed9b WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 2091 8 com.apple.WebCore 0x00007fff398bb45a WebCore::RenderBlock::layout() + 42 9 com.apple.WebCore 0x00007fff398bb0df WebCore::RenderView::layout() + 943 10 com.apple.WebCore 0x00007fff3b2378dc WebCore::FrameViewLayoutContext::layout() + 4844 11 com.apple.WebCore 0x00007fff398b7ca3 WebCore::Document::implicitClose() + 1475 12 com.apple.WebCore 0x00007fff398b7659 WebCore::FrameLoader::checkCompleted() + 553 13 com.apple.WebCore 0x00007fff398b6601 WebCore::FrameLoader::finishedParsing() + 273 14 com.apple.WebCore 0x00007fff398b4721 WebCore::Document::finishedParsing() + 577 15 com.apple.WebCore 0x00007fff398acc1a WebCore::HTMLDocumentParser::prepareToStopParsing() + 234 16 com.apple.WebCore 0x00007fff398ac91c WebCore::HTMLDocumentParser::finish() + 364 <rdar://problem/73826637>

Attachments
Test (181 bytes, text/html) 2021-02-04 17:53 PST, Ryosuke Niwa	no flags	Details
Patch (4.20 KB, patch) 2021-02-12 02:58 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (6.71 KB, patch) 2021-02-12 04:34 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (6.71 KB, patch) 2021-02-12 06:42 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (8.89 KB, patch) 2021-02-15 07:59 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (4.17 KB, patch) 2021-02-18 03:32 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (7.46 KB, patch) 2021-02-22 01:25 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (7.39 KB, patch) 2021-02-24 06:35 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (7.78 KB, patch) 2021-02-24 07:38 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (8.22 KB, patch) 2021-02-24 10:03 PST, Rob Buis	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (8.22 KB, patch) 2021-02-24 10:46 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (8.22 KB, patch) 2021-02-24 11:24 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (4.55 KB, patch) 2021-03-10 06:21 PST, Rob Buis	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (4.92 KB, patch) 2021-03-10 07:59 PST, Rob Buis	no flags	Details Formatted Diff Diff
Patch (4.35 KB, patch) 2021-03-16 02:33 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (4.83 KB, patch) 2021-03-18 03:47 PDT, Rob Buis	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (5.13 KB, patch) 2021-03-18 07:42 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (7.92 KB, patch) 2021-03-22 07:12 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Rob Buis

Comment 1 2021-02-12 02:58:19 PST

Created attachment 420111 [details] Patch

Sergio Villar Senin

Comment 2 2021-02-12 03:05:41 PST

Comment on attachment 420111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420111&action=review > Source/WebCore/rendering/RenderGrid.cpp:586 > +} I think the right fix would be to fix those cases where availableLogicalHeightForPercentageComputation() returns -1 instead of WTF::nullopt > LayoutTests/fast/css-grid-layout/zero-height-crash.html:5 > + grid-template-rows: 0 [x] 0 [x] 0 [x] repeat(auto-fit, 0); The [x] are grid line names, I don't think you need them...

Rob Buis

Comment 3 2021-02-12 04:34:26 PST

Created attachment 420113 [details] Patch

Rob Buis

Comment 4 2021-02-12 06:42:17 PST

Created attachment 420118 [details] Patch

Sergio Villar Senin

Comment 5 2021-02-15 01:37:21 PST

Comment on attachment 420118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420118&action=review Awesome! I'm loving the -1 LayoutUnits removal. Just some nits that you could consider applying before landing. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1057 > + indefiniteSize = LayoutUnit(); You can sill use the ternary operator Optional<LayoutUnit> indefiniteSize = direction() == childInlineDirection : makeOptional(WTFMove(LayoutUnit()) :: WTF::nullopt; > LayoutTests/fast/css-grid-layout/zero-height-crash.html:1 > +<style> Mind adding <!DOCTYPE html> on top ? > LayoutTests/fast/css-grid-layout/zero-height-crash.html:5 > + grid-template-rows: 0 0 0 repeat(auto-fit, 0); Sorry for not spotting it before but the first three rows (0 0 0) shouldn't be needed I guess...

Rob Buis

Comment 6 2021-02-15 07:59:04 PST

Created attachment 420318 [details] Patch

Rob Buis

Comment 7 2021-02-18 03:32:39 PST

Created attachment 420821 [details] Patch

Rob Buis

Comment 8 2021-02-22 01:25:06 PST

Created attachment 421171 [details] Patch

Sergio Villar Senin

Comment 9 2021-02-23 08:12:39 PST

Comment on attachment 421171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421171&action=review Nice! I'll give the r+ once the doubts I have are resolved. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1057 > + indefiniteSize = LayoutUnit(); Removing a -1 yay!!! I'd suggest a couple of changes now that we change it. First I guess we can put it inside the if block as it isn't used outside it. And secondly I think we should rename it to overridingSize as 0 is not an indefinite size, just when we use nullopt. Ah, one last thing, although this is a matter of taste (you can ignore it) I'd prefer to do it in 1 line, auto overridingSize = direction() == childInlineDirection ? makeOptional<LayoutUnit>() : WTF::nullopt; > Source/WebCore/rendering/RenderGrid.cpp:253 > + computeTrackSizesForDefiniteSize(ForRows, std::max<LayoutUnit>(0, availableLogicalHeight(ExcludeMarginBorderPadding))); I wonder why we need this, seems not directly related to the patch?

Darin Adler

Comment 10 2021-02-23 17:40:20 PST

Comment on attachment 421171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421171&action=review >> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1057 >> + indefiniteSize = LayoutUnit(); > > Removing a -1 yay!!! > > I'd suggest a couple of changes now that we change it. First I guess we can put it inside the if block as it isn't used outside it. And secondly I think we should rename it to overridingSize as 0 is not an indefinite size, just when we use nullopt. > > Ah, one last thing, although this is a matter of taste (you can ignore it) I'd prefer to do it in 1 line, > auto overridingSize = direction() == childInlineDirection ? makeOptional<LayoutUnit>() : WTF::nullopt; Maybe 0_lu can be used to make this easier to read in some way? Like makeOptional(0_lu) or indefiniteSize = 0_lu. >> Source/WebCore/rendering/RenderGrid.cpp:253 >> + computeTrackSizesForDefiniteSize(ForRows, std::max<LayoutUnit>(0, availableLogicalHeight(ExcludeMarginBorderPadding))); > > I wonder why we need this, seems not directly related to the patch? Could write std::max(0_lu, ... If this change is needed or valuable, then I guess we’d like a test covering it too.

Rob Buis

Comment 11 2021-02-24 06:35:28 PST

Created attachment 421402 [details] Patch

Rob Buis

Comment 12 2021-02-24 07:38:23 PST

Created attachment 421408 [details] Patch

Rob Buis

Comment 13 2021-02-24 10:03:26 PST

Created attachment 421420 [details] Patch

Rob Buis

Comment 14 2021-02-24 10:46:08 PST

Created attachment 421427 [details] Patch

Rob Buis

Comment 15 2021-02-24 11:24:54 PST

Created attachment 421432 [details] Patch

EWS

Comment 16 2021-02-24 21:59:52 PST

Committed r273470: <https://commits.webkit.org/r273470> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421432 [details].

Aakash Jain

Comment 17 2021-02-25 09:40:53 PST

(In reply to EWS from comment #16) > Committed r273470: <https://commits.webkit.org/r273470> This introduced a failing test zero-height-crash.html on mac-debug-wk1, as also indicated by EWS. History: https://results.webkit.org/?suite=layout-tests&test=fast%2Fcss-grid-layout%2Fzero-height-crash.html

Ryan Haddad

Comment 18 2021-02-25 10:08:51 PST

(In reply to Aakash Jain from comment #17) > (In reply to EWS from comment #16) > > Committed r273470: <https://commits.webkit.org/r273470> > This introduced a failing test zero-height-crash.html on mac-debug-wk1, as > also indicated by EWS. The test is asserting on macOS WK1 and iOS simulator debug bots: ASSERTION FAILED: !availableSize || *availableSize >= 0 ./rendering/RenderGrid.cpp(313) : WebCore::LayoutUnit WebCore::RenderGrid::gridGap(WebCore::GridTrackSizingDirection, Optional<WebCore::LayoutUnit>) const

Ryan Haddad

Comment 19 2021-02-25 10:13:13 PST

Reverted r273470 for reason: New test is asserting on WK1 and iOS bots Committed r273492 (234569@main): <https://commits.webkit.org/234569@main>

Rob Buis

Comment 20 2021-03-10 06:21:00 PST

Created attachment 422824 [details] Patch

Rob Buis

Comment 21 2021-03-10 07:59:27 PST

Created attachment 422834 [details] Patch

Rob Buis

Comment 22 2021-03-16 02:33:03 PDT

Created attachment 423310 [details] Patch

Ryosuke Niwa

Comment 23 2021-03-17 17:58:47 PDT

Comment on attachment 423310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423310&action=review > Source/WebCore/ChangeLog:8 > + Do not allow negative heights (-1) in calculations. Nit: an extra space.

Rob Buis

Comment 24 2021-03-18 03:47:04 PDT

Created attachment 423581 [details] Patch

Sergio Villar Senin

Comment 25 2021-03-18 04:17:30 PDT

Comment on attachment 423581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423581&action=review > Source/WebCore/rendering/RenderGrid.cpp:253 > + computeTrackSizesForDefiniteSize(ForRows, std::max(LayoutUnit(), availableLogicalHeight(ExcludeMarginBorderPadding))); Nit: I think 0_lu is preferred over LayoutUnit() nowadays. Apart from that, computeTrackSizesForDefiniteSize() is called in various places. I think it'd be better to floor the available space inside that method just before computing the gaps. > Source/WebCore/rendering/RenderGrid.cpp:1139 > + LayoutUnit stretchedLogicalHeight = std::max(LayoutUnit(), availableAlignmentSpaceForChildBeforeStretching(GridLayoutFunctions::overridingContainingBlockContentSizeForChild(child, childBlockDirection).value(), child)); Nit: 0_lu. This seems like a different but although I am not strongly against landing this together if it makes sense. In any case I would floor the value inside availableAlignmentSpaceForChildBeforeStretching() because I guess it does not make sense that method returns negative values (which could be possible indeed if margins are big enough). > LayoutTests/fast/css-grid-layout/zero-height-crash.html:8 > + } Does the test properly work if you replace "body, p" by "div" for example. and then just after the <p> you write <div> <div></div> <div>

Rob Buis

Comment 26 2021-03-18 07:42:33 PDT

Created attachment 423601 [details] Patch

Rob Buis

Comment 27 2021-03-18 07:51:41 PDT

Comment on attachment 423581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423581&action=review >> Source/WebCore/rendering/RenderGrid.cpp:253 >> + computeTrackSizesForDefiniteSize(ForRows, std::max(LayoutUnit(), availableLogicalHeight(ExcludeMarginBorderPadding))); > > Nit: I think 0_lu is preferred over LayoutUnit() nowadays. > > Apart from that, computeTrackSizesForDefiniteSize() is called in various places. I think it'd be better to floor the available space inside that method just before computing the gaps. Done. >> Source/WebCore/rendering/RenderGrid.cpp:1139 >> + LayoutUnit stretchedLogicalHeight = std::max(LayoutUnit(), availableAlignmentSpaceForChildBeforeStretching(GridLayoutFunctions::overridingContainingBlockContentSizeForChild(child, childBlockDirection).value(), child)); > > Nit: 0_lu. > > This seems like a different but although I am not strongly against landing this together if it makes sense. > > In any case I would floor the value inside availableAlignmentSpaceForChildBeforeStretching() because I guess it does not make sense that method returns negative values (which could be possible indeed if margins are big enough). Done. >> LayoutTests/fast/css-grid-layout/zero-height-crash.html:8 >> + } > > Does the test properly work if you replace "body, p" by "div" for example. and then just after the <p> you write > <div> > <div></div> > <div> With the patch, it runs fine. Without the patch, but with the ASSERT kept, it hits the ASSERT.

Rob Buis

Comment 28 2021-03-19 08:58:49 PDT

Comment on attachment 423601 [details] Patch @rego, @javiF this is ready for review.

Javier Fernandez

Comment 29 2021-03-22 03:46:42 PDT

Comment on attachment 423601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423601&action=review I think WPT has support for crash tests. Wouldn't make sense to implement it that way ? > Source/WebCore/rendering/RenderGrid.cpp:146 > + m_trackSizingAlgorithm.setup(direction, numTracks(direction, m_grid), TrackSizing, clampedAvailableSpace, freeSpace); In blink we are just clamping the value inside the track sizing algorithm (bear in mind this is an optional value); something like this: SetAvailableSpace( direction, available_space ? available_space.value().ClampNegativeToZero() : available_space); I think that approach is simpler; would you mind consider it ? Another argument for this approach is that in blink, we are not considering the gutters size to determine the available space. This would mean that this is the cause of a different interoperability issue that we might fix in the future, probably removing this code, so that clamping inside the algorithm' setup function makes more sense. > Source/WebCore/rendering/RenderGrid.cpp:1110 > + return std::max(0_lu, gridAreaBreadthForChild - GridLayoutFunctions::marginLogicalSizeForChild(*this, childBlockFlowDirection, child)); I think blink could be affected by this, since the code is not protected against negative height. Could you please verify it and file a bug if that's the case ? This is just a suggestion, but if the test case doesn't crash in blink, it'd be worth investigating why, since perhaps we solved the issue there already and we could perhaps consider that solution for WebKit as well.

Rob Buis

Comment 30 2021-03-22 03:57:09 PDT

Comment on attachment 423601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423601&action=review >> Source/WebCore/rendering/RenderGrid.cpp:1110 >> + return std::max(0_lu, gridAreaBreadthForChild - GridLayoutFunctions::marginLogicalSizeForChild(*this, childBlockFlowDirection, child)); > > I think blink could be affected by this, since the code is not protected against negative height. Could you please verify it and file a bug if that's the case ? > > This is just a suggestion, but if the test case doesn't crash in blink, it'd be worth investigating why, since perhaps we solved the issue there already and we could perhaps consider that solution for WebKit as well. The same test crashes in blink, with LayoutNG enabled or not: [13566:1:0322/115334.626912:FATAL:layout_grid.cc(486)] Check failed: gap_accumulator >= gap ("-3" vs. "-1") I think we have to verify first if we can share the testcase with blink though.

Rob Buis

Comment 31 2021-03-22 07:12:03 PDT

Created attachment 423880 [details] Patch

Rob Buis

Comment 32 2021-03-22 07:13:20 PDT

Comment on attachment 423601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423601&action=review >> Source/WebCore/rendering/RenderGrid.cpp:146 >> + m_trackSizingAlgorithm.setup(direction, numTracks(direction, m_grid), TrackSizing, clampedAvailableSpace, freeSpace); > > In blink we are just clamping the value inside the track sizing algorithm (bear in mind this is an optional value); something like this: > > SetAvailableSpace( > direction, available_space ? available_space.value().ClampNegativeToZero() > : available_space); > > I think that approach is simpler; would you mind consider it ? > Another argument for this approach is that in blink, we are not considering the gutters size to determine the available space. This would mean that this is the cause of a different interoperability issue that we might fix in the future, probably removing this code, so that clamping inside the algorithm' setup function makes more sense. I implemented that, but it does require moving more code into setup(), including the gutters size calculation.

Ryosuke Niwa

Comment 33 2021-03-22 22:56:24 PDT

(In reply to Rob Buis from comment #30) > Comment on attachment 423601 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423601&action=review > > >> Source/WebCore/rendering/RenderGrid.cpp:1110 > >> + return std::max(0_lu, gridAreaBreadthForChild - GridLayoutFunctions::marginLogicalSizeForChild(*this, childBlockFlowDirection, child)); > > > > I think blink could be affected by this, since the code is not protected against negative height. Could you please verify it and file a bug if that's the case ? > > > > This is just a suggestion, but if the test case doesn't crash in blink, it'd be worth investigating why, since perhaps we solved the issue there already and we could perhaps consider that solution for WebKit as well. > > The same test crashes in blink, with LayoutNG enabled or not: > [13566:1:0322/115334.626912:FATAL:layout_grid.cc(486)] Check failed: > gap_accumulator >= gap ("-3" vs. "-1") > > I think we have to verify first if we can share the testcase with blink > though. To an extent once the patch is landed with a test, it's okay for someone else to import the test case into Blink. However, in that case, the test's credit needs to be attributed to Apple.

Javier Fernandez

Comment 34 2021-03-23 15:45:51 PDT

Comment on attachment 423880 [details] Patch r=me

EWS

Comment 35 2021-03-24 00:44:24 PDT

Committed r274933: <https://commits.webkit.org/r274933> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423880 [details].

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