From 4f855286d7d3bf55be0a5c66709b5f86cd9c3743 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 5 Feb 2025 14:13:33 +0100 Subject: [PATCH] LibWeb: Clamp layout content sizes to a max value instead of crashing We've historically asserted that no "saturated" size values end up as final metrics for boxes in layout. This always had a chance of producing false positives, since you can trivially create extremely large boxes with CSS. The reason we had those assertions was to catch bugs in our own engine code where we'd incorrectly end up with non-finite values in layout algorithms. At this point, we've found and fixed all known bugs of that nature, and what remains are a bunch of false positives on pages that create very large scrollable areas, iframes etc. So, let's change it! We now clamp content width and height of boxes to 17895700 pixels, apparently the same cap as Firefox uses. There's also the issue of calc() being able to produce non-finite values. Note that we don't clamp the result of calc() directly, but instead just clamp values when assigning them to content sizes. Fixes #645. Fixes #1236. Fixes #1249. Fixes #1908. Fixes #3057. --- Libraries/LibWeb/HTML/Parser/HTMLParser.cpp | 5 +-- Libraries/LibWeb/Layout/FormattingContext.cpp | 34 +++---------------- Libraries/LibWeb/Layout/LayoutState.cpp | 31 ++++++++--------- Libraries/LibWeb/Layout/LayoutState.h | 7 ++++ Libraries/LibWeb/PixelUnits.h | 3 ++ .../CSS/height-calc-infinity-result.html | 5 +++ .../Crash/CSS/width-calc-infinity-result.html | 5 +++ 7 files changed, 40 insertions(+), 50 deletions(-) create mode 100644 Tests/LibWeb/Crash/CSS/height-calc-infinity-result.html create mode 100644 Tests/LibWeb/Crash/CSS/width-calc-infinity-result.html diff --git a/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp b/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp index 53dde33bcc6..039baef4041 100644 --- a/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp +++ b/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp @@ -4896,10 +4896,7 @@ RefPtr parse_dimension_value(StringView string) } auto integer_value = number_string.string_view().to_number(); - // NOTE: This is apparently the largest value allowed by Firefox. - static float max_dimension_value = 17895700; - - float value = min(*integer_value, max_dimension_value); + float value = min(*integer_value, CSSPixels::max_dimension_value); // 6. If position is past the end of input, then return value as a length. if (position == input.end()) diff --git a/Libraries/LibWeb/Layout/FormattingContext.cpp b/Libraries/LibWeb/Layout/FormattingContext.cpp index c678bf09b71..c09a7aca0fb 100644 --- a/Libraries/LibWeb/Layout/FormattingContext.cpp +++ b/Libraries/LibWeb/Layout/FormattingContext.cpp @@ -1466,14 +1466,7 @@ CSSPixels FormattingContext::calculate_min_content_width(Layout::Box const& box) context->run(AvailableSpace(available_width, available_height)); - cache.min_content_width = context->automatic_content_width(); - - if (cache.min_content_width->might_be_saturated()) { - // HACK: If layout calculates a non-finite result, something went wrong. Force it to zero and log a little whine. - dbgln("FIXME: Calculated non-finite min-content width for {}", box.debug_description()); - cache.min_content_width = 0; - } - + cache.min_content_width = clamp_to_max_dimension_value(context->automatic_content_width()); return *cache.min_content_width; } @@ -1506,14 +1499,7 @@ CSSPixels FormattingContext::calculate_max_content_width(Layout::Box const& box) context->run(AvailableSpace(available_width, available_height)); - cache.max_content_width = context->automatic_content_width(); - - if (cache.max_content_width->might_be_saturated()) { - // HACK: If layout calculates a non-finite result, something went wrong. Force it to zero and log a little whine. - dbgln("FIXME: Calculated non-finite max-content width for {}", box.debug_description()); - cache.max_content_width = 0; - } - + cache.max_content_width = clamp_to_max_dimension_value(context->automatic_content_width()); return *cache.max_content_width; } @@ -1550,13 +1536,7 @@ CSSPixels FormattingContext::calculate_min_content_height(Layout::Box const& box context->run(AvailableSpace(AvailableSize::make_definite(width), AvailableSize::make_min_content())); - auto min_content_height = context->automatic_content_height(); - if (min_content_height.might_be_saturated()) { - // HACK: If layout calculates a non-finite result, something went wrong. Force it to zero and log a little whine. - dbgln("FIXME: Calculated non-finite min-content height for {}", box.debug_description()); - min_content_height = 0; - } - + auto min_content_height = clamp_to_max_dimension_value(context->automatic_content_height()); if (auto* cache_slot = get_cache_slot()) { *cache_slot = min_content_height; } @@ -1594,13 +1574,7 @@ CSSPixels FormattingContext::calculate_max_content_height(Layout::Box const& box context->run(AvailableSpace(AvailableSize::make_definite(width), AvailableSize::make_max_content())); - auto max_content_height = context->automatic_content_height(); - - if (max_content_height.might_be_saturated()) { - // HACK: If layout calculates a non-finite result, something went wrong. Force it to zero and log a little whine. - dbgln("FIXME: Calculated non-finite max-content height for {}", box.debug_description()); - max_content_height = 0; - } + auto max_content_height = clamp_to_max_dimension_value(context->automatic_content_height()); if (auto* cache_slot = get_cache_slot()) { *cache_slot = max_content_height; diff --git a/Libraries/LibWeb/Layout/LayoutState.cpp b/Libraries/LibWeb/Layout/LayoutState.cpp index 431570493d0..3f0169a12b0 100644 --- a/Libraries/LibWeb/Layout/LayoutState.cpp +++ b/Libraries/LibWeb/Layout/LayoutState.cpp @@ -535,13 +535,14 @@ void LayoutState::UsedValues::set_node(NodeWithStyle& node, UsedValues const* co || node.parent()->display().is_flow_inside())) { if (containing_block_has_definite_size) { CSSPixels available_width = containing_block_used_values->content_width(); - resolved_definite_size = available_width + resolved_definite_size = clamp_to_max_dimension_value( + available_width - margin_left - margin_right - padding_left - padding_right - border_left - - border_right; + - border_right); return true; } return false; @@ -559,19 +560,19 @@ void LayoutState::UsedValues::set_node(NodeWithStyle& node, UsedValues const* co auto containing_block_size_as_length = width ? containing_block_used_values->content_width() : containing_block_used_values->content_height(); context.percentage_basis = CSS::Length::make_px(containing_block_size_as_length); } - resolved_definite_size = adjust_for_box_sizing(size.calculated().resolve_length(context)->to_px(node), size, width); + resolved_definite_size = clamp_to_max_dimension_value(adjust_for_box_sizing(size.calculated().resolve_length(context)->to_px(node), size, width)); return true; } if (size.is_length()) { VERIFY(!size.is_auto()); // This should have been covered by the Size::is_auto() branch above. - resolved_definite_size = adjust_for_box_sizing(size.length().to_px(node), size, width); + resolved_definite_size = clamp_to_max_dimension_value(adjust_for_box_sizing(size.length().to_px(node), size, width)); return true; } if (size.is_percentage()) { if (containing_block_has_definite_size) { auto containing_block_size = width ? containing_block_used_values->content_width() : containing_block_used_values->content_height(); - resolved_definite_size = adjust_for_box_sizing(containing_block_size.scaled(size.percentage().as_fraction()), size, width); + resolved_definite_size = clamp_to_max_dimension_value(adjust_for_box_sizing(containing_block_size.scaled(size.percentage().as_fraction()), size, width)); return true; } return false; @@ -600,10 +601,10 @@ void LayoutState::UsedValues::set_node(NodeWithStyle& node, UsedValues const* co if (m_has_definite_width && m_has_definite_height) { // Both width and height are definite. } else if (m_has_definite_width) { - m_content_height = m_content_width / *aspect_ratio; + m_content_height = clamp_to_max_dimension_value(m_content_width / *aspect_ratio); m_has_definite_height = true; } else if (m_has_definite_height) { - m_content_width = m_content_height * *aspect_ratio; + m_content_width = clamp_to_max_dimension_value(m_content_height * *aspect_ratio); m_has_definite_width = true; } } @@ -611,28 +612,27 @@ void LayoutState::UsedValues::set_node(NodeWithStyle& node, UsedValues const* co if (m_has_definite_width) { if (has_definite_min_width) - m_content_width = max(min_width, m_content_width); + m_content_width = clamp_to_max_dimension_value(max(min_width, m_content_width)); if (has_definite_max_width) - m_content_width = min(max_width, m_content_width); + m_content_width = clamp_to_max_dimension_value(min(max_width, m_content_width)); } if (m_has_definite_height) { if (has_definite_min_height) - m_content_height = max(min_height, m_content_height); + m_content_height = clamp_to_max_dimension_value(max(min_height, m_content_height)); if (has_definite_max_height) - m_content_height = min(max_height, m_content_height); + m_content_height = clamp_to_max_dimension_value(min(max_height, m_content_height)); } } void LayoutState::UsedValues::set_content_width(CSSPixels width) { - VERIFY(!width.might_be_saturated()); if (width < 0) { // Negative widths are not allowed in CSS. We have a bug somewhere! Clamp to 0 to avoid doing too much damage. dbgln_if(LIBWEB_CSS_DEBUG, "FIXME: Layout calculated a negative width for {}: {}", m_node->debug_description(), width); width = 0; } - m_content_width = width; + m_content_width = clamp_to_max_dimension_value(width); // FIXME: We should not do this! Definiteness of widths should be determined early, // and not changed later (except for some special cases in flex layout..) m_has_definite_width = true; @@ -640,18 +640,17 @@ void LayoutState::UsedValues::set_content_width(CSSPixels width) void LayoutState::UsedValues::set_content_height(CSSPixels height) { - VERIFY(!height.might_be_saturated()); if (height < 0) { // Negative heights are not allowed in CSS. We have a bug somewhere! Clamp to 0 to avoid doing too much damage. dbgln_if(LIBWEB_CSS_DEBUG, "FIXME: Layout calculated a negative height for {}: {}", m_node->debug_description(), height); height = 0; } - m_content_height = height; + m_content_height = clamp_to_max_dimension_value(height); } void LayoutState::UsedValues::set_temporary_content_width(CSSPixels width) { - m_content_width = width; + m_content_width = clamp_to_max_dimension_value(width); } void LayoutState::UsedValues::set_temporary_content_height(CSSPixels height) diff --git a/Libraries/LibWeb/Layout/LayoutState.h b/Libraries/LibWeb/Layout/LayoutState.h index b20f5dada59..0d8a9f99552 100644 --- a/Libraries/LibWeb/Layout/LayoutState.h +++ b/Libraries/LibWeb/Layout/LayoutState.h @@ -244,4 +244,11 @@ private: void resolve_relative_positions(); }; +inline CSSPixels clamp_to_max_dimension_value(CSSPixels value) +{ + if (value.might_be_saturated()) + return CSSPixels(CSSPixels::max_dimension_value); + return value; +} + } diff --git a/Libraries/LibWeb/PixelUnits.h b/Libraries/LibWeb/PixelUnits.h index 65aa2663c7e..56029327394 100644 --- a/Libraries/LibWeb/PixelUnits.h +++ b/Libraries/LibWeb/PixelUnits.h @@ -66,6 +66,9 @@ public: static constexpr i32 max_integer_value = NumericLimits::max() >> fractional_bits; static constexpr i32 min_integer_value = NumericLimits::min() >> fractional_bits; + // NOTE: This is apparently the largest value allowed by Firefox. Probably enough for us as well. + static constexpr float max_dimension_value = 17895700; + constexpr CSSPixels() = default; template constexpr CSSPixels(I value) diff --git a/Tests/LibWeb/Crash/CSS/height-calc-infinity-result.html b/Tests/LibWeb/Crash/CSS/height-calc-infinity-result.html new file mode 100644 index 00000000000..37a358617e7 --- /dev/null +++ b/Tests/LibWeb/Crash/CSS/height-calc-infinity-result.html @@ -0,0 +1,5 @@ + diff --git a/Tests/LibWeb/Crash/CSS/width-calc-infinity-result.html b/Tests/LibWeb/Crash/CSS/width-calc-infinity-result.html new file mode 100644 index 00000000000..e6c37b3df9a --- /dev/null +++ b/Tests/LibWeb/Crash/CSS/width-calc-infinity-result.html @@ -0,0 +1,5 @@ +