Created attachment 457704 [details]
Patch

Comment on attachment 457704 [details]
Patch

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

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:988
> +static std::optional<LayoutUnit> getHeightForLineCount(const RenderBlockFlow& block, size_t lineCount, bool includeBottom, size_t& count)

If we are changing the return type, it would be better to return a structure with both the count and the height, instead of using a return value plus an out argument.

But maybe count is an in/out argument?

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1003
> +                    return *height + obj->y() + (includeBottom ? (block.borderBottom() + block.paddingBottom()) : 0_lu);

I am not familiar with how we avoid overflow in math like this; do we stay far from the limits of the LayoutUnit type to avoid it? Is there a guarantee that this expression won’t overflow?

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1066
> +    size_t numVisibleLines = lineClamp.isPercentage() ? std::max<size_t>(1, (maxLineCount + 1) * lineClamp.value() / 100) : lineClamp.value();

What prevents overflow in the expression "(maxLineCount + 1) * lineClamp.value() / 100"?

(In reply to Darin Adler from comment #2)
> Comment on attachment 457704 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457704&action=review
> 
> > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:988
> > +static std::optional<LayoutUnit> getHeightForLineCount(const RenderBlockFlow& block, size_t lineCount, bool includeBottom, size_t& count)
> 
> If we are changing the return type, it would be better to return a structure
> with both the count and the height, instead of using a return value plus an
> out argument.
> 
> But maybe count is an in/out argument?
I agree, I don't particularly like this setup.
'count' is an in/out argument.


> > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1003
> > +                    return *height + obj->y() + (includeBottom ? (block.borderBottom() + block.paddingBottom()) : 0_lu);
> 
> I am not familiar with how we avoid overflow in math like this; do we stay
> far from the limits of the LayoutUnit type to avoid it? Is there a guarantee
> that this expression won’t overflow?
The last time I looked LayoutUnit used saturated arithmetic.

> 
> > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1066
> > +    size_t numVisibleLines = lineClamp.isPercentage() ? std::max<size_t>(1, (maxLineCount + 1) * lineClamp.value() / 100) : lineClamp.value();
> 
> What prevents overflow in the expression "(maxLineCount + 1) *
> lineClamp.value() / 100"?
Yeah, nothing really. In practice I don't think we could produce this high amount of lines, but it should not stop us from trying to avoid arithmetic overflow.

Created attachment 457756 [details]
Patch

Committed r292948 (249713@main): <https://commits.webkit.org/249713@main>

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

<rdar://problem/91861450>