Crash in IndefiniteSizeStrategy::recomputeUsedFlexFractionIfNeeded when min-size can not be resolved

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[zalan]

<rdar://problem/56685237>

[zalan]

Created attachment 396601 [details]
Patch

[zalan]

I haven't checked the grid spec so this might need to be turned into some kind of error state by returning false from this function. In general, or at least in block layout such circular dependencies could be just resolved by using the initial 0 value.

[Manuel Rego Casasnovas]

Comment on attachment 396601 [details]
Patch

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

Thanks for fixing this, LGTM.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1017
> +    freeSpace = std::max(freeSpace, minSize.valueOr(0_lu)) - renderGrid->guttersSize(grid, ForRows, 0, grid.numTracks(ForRows), availableSpace());

For heights we usually go with -1 when they're unknown (check just 2 lines above), but this is not going to change anything in the end if we use -1 or 0. If any of them is the max, then findFrUnitSize will return 0 (due to the early return in GridTrackSizingAlgorithm::findFrUnitSize()) so the final result would be the same.

> LayoutTests/fast/css-grid-layout/crash-when-min-height-cant-be-resolved.html:6
> +    grid: 1fr / auto-flow;

Nit: This line can be simplified to "grid-template-rows: 1fr" as it's already enough to cause the crash.

[zalan]

(In reply to Manuel Rego Casasnovas from comment #3)
> Comment on attachment 396601 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396601&action=review
> 
> Thanks for fixing this, LGTM.
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1017
> > +    freeSpace = std::max(freeSpace, minSize.valueOr(0_lu)) - renderGrid->guttersSize(grid, ForRows, 0, grid.numTracks(ForRows), availableSpace());
> 
> For heights we usually go with -1 when they're unknown (check just 2 lines
> above), but this is not going to change anything in the end if we use -1 or
> 0. If any of them is the max, then findFrUnitSize will return 0 (due to the
> early return in GridTrackSizingAlgorithm::findFrUnitSize()) so the final
> result would be the same.
Yeah I saw that logic. I don't particularly like special -1 values, if the constraint can be resolved with the initial value, I'd rather go with that.
Thanks for the review.

[zalan]

Created attachment 396660 [details]
Patch

[EWS]

Committed r260196: <https://trac.webkit.org/changeset/260196>

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