From 85145cad40821a072a047eb0bb3dccaefcfef049 Mon Sep 17 00:00:00 2001 From: Sammy Gill Date: Sun, 26 Feb 2023 08:36:16 -0800 Subject: [PATCH] Cherry-pick 260851@main (e6a31953a657). https://bugs.webkit.org/show_bug.cgi?id=252589 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RenderReplaced::computeAspectRatioInformationForRenderBox does not need to call into RenderBox::computeReplacedLogicalWidth https://bugs.webkit.org/show_bug.cgi?id=252589 rdar://105489410 Reviewed by Alan Baradlay. The call to RenderBox::computeReplacedLogicalWidth from RenderReplaced::computeAspectRatioInformationForRenderBox when the object was a video with a default object size was extraneous and can be safely removed without changing any functionality. It could also result in a stack overflow in certain conditions. At this point we know the intrinsic sizes of the object anyway since it is a video with a default object size (300px x 150px). This is because: 1.) RenderBox::computeReplacedLogicalWidth would call into computeReplacedLogicalWidthUsing(MainOrPreferredSize, style().logicalWidth()) com compute the width value to use 2.) The caller (RenderReplaced::computeReplacedLogicalWidth in this case) will only use the width value of style().logicalWidth().isAuto() returns true. 3.) If this is true RenderBox::computeReplacedLogicalWidth would have returned the intrinsic width but constrained to min/max sizes. However, the caller constrains the intrinsic width anyway. 4.) If this is false (style().logicalWidth().isAuto() returns false), then the caller (RenderReplaced::computeReplacedLogicalWidth) will use the intrinsic width anyway by calling into intrinsicLogicalWidth. We can also take advantage of the existing logic to constrain the intrinsic sizes in each dimension by transferred constraints from the opposite one. Here is an example of when this extra call could cause a stack overflow: WebCore::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth(WebCore::LayoutUnit, WebCore::ShouldComputePreferred) const WebCore::RenderBox::computeReplacedLogicalWidth(WebCore::ShouldComputePreferred) const WebCore::RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes(WebCore::RenderBox*, WebCore::FloatSize&, WebCore::FloatSize&) const WebCore::RenderReplaced::computeReplacedLogicalHeight(std::__1::optional) const WebCore::RenderImage::computeReplacedLogicalHeight(std::__1::optional) const WebCore::RenderBox::computePositionedLogicalHeightReplaced(WebCore::RenderBox::LogicalExtentComputedValues&) const WebCore::RenderBox::computePositionedLogicalHeight(WebCore::RenderBox::LogicalExtentComputedValues&) const WebCore::RenderBox::computeLogicalHeight(WebCore::LayoutUnit, WebCore::LayoutUnit) const WebCore::RenderBox::computeLogicalWidthFromAspectRatioInternal() const WebCore::RenderBox::computeIntrinsicLogicalWidthUsing(WebCore::Length, WebCore::LayoutUnit, WebCore::LayoutUnit) const WebCore::RenderBox::computeReplacedLogicalWidthUsing(WebCore::SizeType, WebCore::Length) const WebCore::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth(WebCore::LayoutUnit, WebCore::ShouldComputePreferred) const WebCore::RenderVideo::computeReplacedLogicalWidth(WebCore::ShouldComputePreferred) const WebCore::RenderBox::computePositionedLogicalWidthReplaced(WebCore::RenderBox::LogicalExtentComputedValues&) const WebCore::RenderBox::computePositionedLogicalWidth(WebCore::RenderBox::LogicalExtentComputedValues&, WebCore::RenderFragmentContainer*) const WebCore::RenderBox::computeLogicalWidthInFragment(WebCore::RenderBox::LogicalExtentComputedValues&, WebCore::RenderFragmentContainer*) const WebCore::RenderBox::updateLogicalWidth() WebCore::RenderReplaced::layout() WebCore::RenderImage::layout() WebCore::RenderMedia::layout() WebCore::RenderVideo::layout() After we have computed the width for the video, we call RenderBox::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth to restrict it within any min and max constraints. This eventually calls into RenderBox::computeIntrinsicLogicalWidthUsing to compute the min-width using MinContent as the width type. The container-type: inline-size; and inset: 0 auto; properties cause RenderBox::shouldComputeLogicalWidthFromAspectRatio to return true because the call to shouldComputeLogicalWidthFromAspectRatioAndInsets returns true. This last method returns true because: 1.) Applying the inline size containment causes hasConstrainedWidth to be false when it would normally be true from the video’s intrinsic width. 2.) hasConstrainedHeight gets set to false because inset causes the logical top and property values to get set to a fixed value 3.) The final call to style.logicalHeight().isAuto() returns true Due to these series of events we end up calling computeLogicalWidthFromAspectRatioInternal inside RenderBox::computeIntrinsicLogicalWidthUsing. This eventually recurses its way back into RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth through the reset of the stack trace. * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-inline-size-containment-no-crash.html: Added. This test currently crashes due to a different reason (webkit.org/b/252594), but is still useful to this patch as it was causing the stack overflow. * LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-intrinsic-width-height.html: One of the test cases was incorrectly changed in a previous patch and is being restored to its previous version. The height for this test should be 150px and not 300px because it falls under the case: If 'height' and 'width' both have computed values of 'auto' and the element also has an intrinsic height, then that intrinsic height is the used value of 'height'. https://www.w3.org/TR/CSS22/visudet.html#inline-replaced-height * LayoutTests/platform/gtk/TestExpectations: * LayoutTests/platform/wpe/TestExpectations: * Source/WebCore/rendering/RenderReplaced.cpp: (WebCore::RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes const): Canonical link: https://commits.webkit.org/260851@main --- LayoutTests/TestExpectations | 1 + ...ght-constrained-by-max-width-expected.html | 10 ++++++ ...bject-height-constrained-by-max-width.html | 22 +++++++++++++ ...th-constrained-by-max-height-expected.html | 10 ++++++ ...bject-width-constrained-by-max-height.html | 22 +++++++++++++ ...ideo-inline-size-containment-no-crash.html | 19 +++++++++++ .../video-intrinsic-width-height.html | 2 +- LayoutTests/platform/gtk/TestExpectations | 2 ++ LayoutTests/platform/wpe/TestExpectations | 3 ++ Source/WebCore/rendering/RenderReplaced.cpp | 33 ++++++++----------- 10 files changed, 103 insertions(+), 21 deletions(-) create mode 100644 LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html create mode 100644 LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html create mode 100644 LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height-expected.html create mode 100644 LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height.html create mode 100644 LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-inline-size-containment-no-crash.html diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations index 1731993f89db..2f7b6f69ef67 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -827,6 +827,7 @@ imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/propagate-t imported/w3c/web-platform-tests/html/rendering/widgets/the-select-element/option-checked-styling.html [ ImageOnlyFailure ] imported/w3c/web-platform-tests/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/legend-painting-order.html [ ImageOnlyFailure ] imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/input-image-inline-alt.html [ ImageOnlyFailure ] +webkit.org/b/252594 imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-inline-size-containment-no-crash.html [ Skip ] imported/w3c/web-platform-tests/html/rendering/replaced-elements/embedded-content/cross-domain-iframe.sub.html [ ImageOnlyFailure ] imported/w3c/web-platform-tests/html/rendering/replaced-elements/embedded-content/tall-cross-domain-iframe-in-scrolled.sub.html [ ImageOnlyFailure ] imported/w3c/web-platform-tests/html/rendering/the-details-element/details-page-break-after-1-print.html [ ImageOnlyFailure ] diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html b/LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html new file mode 100644 index 000000000000..0242cf512c22 --- /dev/null +++ b/LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html @@ -0,0 +1,10 @@ + + + + + + +

Test passes if there is a filled green square.

+
+ + diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html b/LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html new file mode 100644 index 000000000000..33a4f381e385 --- /dev/null +++ b/LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html @@ -0,0 +1,22 @@ + + + + + + + + + + + +

Test passes if there is a filled green square.

+