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>
Created attachment 420111 [details] Patch
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...
Created attachment 420113 [details] Patch
Created attachment 420118 [details] Patch
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...
Created attachment 420318 [details] Patch
Created attachment 420821 [details] Patch
Created attachment 421171 [details] Patch
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?
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.
Created attachment 421402 [details] Patch
Created attachment 421408 [details] Patch
Created attachment 421420 [details] Patch
Created attachment 421427 [details] Patch
Created attachment 421432 [details] Patch
Committed r273470: <https://commits.webkit.org/r273470> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421432 [details].
(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
(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
Reverted r273470 for reason: New test is asserting on WK1 and iOS bots Committed r273492 (234569@main): <https://commits.webkit.org/234569@main>
Created attachment 422824 [details] Patch
Created attachment 422834 [details] Patch
Created attachment 423310 [details] Patch
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.
Created attachment 423581 [details] Patch
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>
Created attachment 423601 [details] Patch
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.
Comment on attachment 423601 [details] Patch @rego, @javiF this is ready for review.
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.
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.
Created attachment 423880 [details] Patch
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.
(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.
Comment on attachment 423880 [details] Patch r=me
Committed r274933: <https://commits.webkit.org/r274933> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423880 [details].