M96: Fix "MinMax after Layout" crash during baseline calculation
Fixing a bug where a grid item with baseline alignment needs to be laid
out during `ComputeMinMaxSizes`, causing a "MinMax after Layout" crash.
Refactoring all uses of `Layout` that could be affected within a MinMax
pass of the grid layout algorithm to use `NGDisableSideEffectsScope`.
See https://docs.google.com/document/d/1HkMwnfr-JbP2SE0ktYalkeAutwciuy-GftLs3TOnBcI
(cherry picked from commit cf4540e4c72e187be37141cb520381e88658867a)
Bug: 1266688
Change-Id: I84e7f0a372d376ef3cfa4a6767df78ac729fc37f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3276669
Commit-Queue: Ethan Jimenez <ethavar@microsoft.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Cr-Original-Commit-Position: refs/heads/main@{#942018}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3314356
Commit-Queue: Daniel Libby <dlibby@microsoft.com>
Cr-Commit-Position: refs/branch-heads/4664@{#1217}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}
diff --git a/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.cc b/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.cc
index 8f6d93b..c015857 100644
--- a/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.cc
+++ b/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.cc
@@ -197,7 +197,8 @@
// sizing.
bool needs_additional_pass = false;
if (grid_properties.HasBaseline(kForColumns)) {
- CalculateAlignmentBaselines(kForColumns, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForColumns, /* is_min_max_pass */ false,
+ &grid_geometry, &grid_items,
&needs_additional_pass);
}
@@ -209,7 +210,8 @@
&needs_additional_pass, &has_block_size_dependent_item);
if (grid_properties.HasBaseline(kForRows)) {
- CalculateAlignmentBaselines(kForRows, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForRows, /* is_min_max_pass */ false,
+ &grid_geometry, &grid_items,
&needs_additional_pass);
}
@@ -235,7 +237,8 @@
// tracks, re-run the track sizing algorithm for both dimensions.
if (needs_additional_pass) {
if (grid_properties.HasBaseline(kForColumns)) {
- CalculateAlignmentBaselines(kForColumns, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForColumns, /* is_min_max_pass */ false,
+ &grid_geometry, &grid_items,
&unused_needs_additional_pass);
}
@@ -247,7 +250,8 @@
&unused_needs_additional_pass);
if (grid_properties.HasBaseline(kForRows)) {
- CalculateAlignmentBaselines(kForRows, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForRows, /* is_min_max_pass */ false,
+ &grid_geometry, &grid_items,
&unused_needs_additional_pass);
}
@@ -259,11 +263,13 @@
}
if (grid_properties.HasBaseline(kForColumns)) {
- CalculateAlignmentBaselines(kForColumns, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForColumns, /* is_min_max_pass */ false,
+ &grid_geometry, &grid_items,
&unused_needs_additional_pass);
}
if (grid_properties.HasBaseline(kForRows)) {
- CalculateAlignmentBaselines(kForRows, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForRows, /* is_min_max_pass */ false,
+ &grid_geometry, &grid_items,
&unused_needs_additional_pass);
}
DCHECK(!unused_needs_additional_pass);
@@ -460,7 +466,8 @@
bool needs_additional_pass = false;
if (grid_properties.HasBaseline(kForColumns)) {
- CalculateAlignmentBaselines(kForColumns, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForColumns, /* is_min_max_pass */ true,
+ &grid_geometry, &grid_items,
&needs_additional_pass);
}
@@ -471,7 +478,8 @@
&needs_additional_pass, &has_block_size_dependent_item);
if (grid_properties.HasBaseline(kForRows)) {
- CalculateAlignmentBaselines(kForRows, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForRows, /* is_min_max_pass */ true,
+ &grid_geometry, &grid_items,
&needs_additional_pass);
}
@@ -499,7 +507,8 @@
if (needs_additional_pass) {
if (grid_properties.HasBaseline(kForColumns)) {
- CalculateAlignmentBaselines(kForColumns, &grid_geometry, &grid_items,
+ CalculateAlignmentBaselines(kForColumns, /* is_min_max_pass */ true,
+ &grid_geometry, &grid_items,
&unused_needs_additional_pass);
}
@@ -999,6 +1008,23 @@
BorderScrollbarPadding().block_end;
}
+namespace {
+
+scoped_refptr<const NGLayoutResult> LayoutNodeForMeasure(
+ const NGBlockNode& node,
+ const NGConstraintSpace& constraint_space,
+ const bool is_min_max_pass) {
+ // Disable side effects during MinMax computation to avoid potential "MinMax
+ // after layout" crashes. This is not necessary during the layout pass, and
+ // would have a negative impact on performance if used there.
+ absl::optional<NGDisableSideEffectsScope> disable_side_effects;
+ if (is_min_max_pass && !node.GetLayoutBox()->NeedsLayout())
+ disable_side_effects.emplace();
+ return node.Layout(constraint_space);
+}
+
+} // namespace
+
LayoutUnit NGGridLayoutAlgorithm::ContributionSizeForGridItem(
const GridGeometry& grid_geometry,
const GridItemData& grid_item,
@@ -1057,13 +1083,6 @@
if (is_for_columns && !is_parallel && has_block_size_dependent_item)
*has_block_size_dependent_item = true;
- // Disable side effects during MinMax computation to avoid potential
- // "MinMax after layout" crashes. This is not necessary during layout, and
- // will have a negative impact on performance if used there.
- absl::optional<NGDisableSideEffectsScope> disable_side_effects;
- if (is_min_max_pass && !node.GetLayoutBox()->NeedsLayout())
- disable_side_effects.emplace();
-
scoped_refptr<const NGLayoutResult> result;
if (!is_parallel && space.AvailableSize().inline_size == kIndefiniteSize) {
// The only case where we will have an indefinite block size is for the
@@ -1076,13 +1095,14 @@
const auto fallback_space = CreateConstraintSpaceForMeasure(
grid_geometry, grid_item, track_direction,
/* opt_fixed_block_size */ MinMaxContentSizes().max_size);
- result = node.Layout(fallback_space);
+
+ result = LayoutNodeForMeasure(node, fallback_space, is_min_max_pass);
// TODO(ikilpatrick): This check is potentially too broad, i.e. a fixed
// inline size with no %-padding doesn't need the additional pass.
*needs_additional_pass = true;
} else {
- result = node.Layout(space);
+ result = LayoutNodeForMeasure(node, space, is_min_max_pass);
}
NGBoxFragment fragment(
@@ -1798,39 +1818,9 @@
TrackSpanProperties::kHasAutoMinimumTrack);
}
-bool NGGridLayoutAlgorithm::CanLayoutGridItem(
- const GridItemData& grid_item,
- const NGConstraintSpace& space,
- const GridTrackSizingDirection track_direction) const {
- // Baseline eligibility based on layout only applies to flex and intrinsic
- // tracks.
- const auto& item_style = grid_item.node.Style();
- const bool logical_width_depends_on_container =
- item_style.LogicalWidth().IsPercentOrCalc() ||
- item_style.LogicalMinWidth().IsPercentOrCalc() ||
- item_style.LogicalMaxWidth().IsPercentOrCalc();
-
- const bool logical_height_depends_on_container =
- item_style.LogicalHeight().IsPercentOrCalc() ||
- item_style.LogicalMinHeight().IsPercentOrCalc() ||
- item_style.LogicalMaxHeight().IsPercentOrCalc() ||
- item_style.LogicalHeight().IsAuto();
-
- // TODO(kschmi) - this should be using 'BlockLengthUnresolvable' and
- // 'InlineLengthUnresolvable', however those are a too strict and don't
- // end up laying out enough grid items.
- const bool can_layout_block_axis =
- space.AvailableSize().block_size != kIndefiniteSize ||
- !logical_height_depends_on_container;
- const bool can_layout_inline_axis =
- space.AvailableSize().inline_size != kIndefiniteSize ||
- !logical_width_depends_on_container;
-
- return can_layout_inline_axis && can_layout_block_axis;
-}
-
void NGGridLayoutAlgorithm::CalculateAlignmentBaselines(
const GridTrackSizingDirection track_direction,
+ const bool is_min_max_pass,
GridGeometry* grid_geometry,
GridItems* grid_items,
bool* needs_additional_pass) const {
@@ -1847,8 +1837,32 @@
grid_geometry->minor_block_baselines.Fill(LayoutUnit::Min());
}
- // TODO(kschmi): Skip this loop (or method) entirely if we don't have any
- // baseline-aligned grid-items.
+ auto CanLayoutGridItem = [](const ComputedStyle& item_style,
+ const NGConstraintSpace& space) -> bool {
+ const bool logical_width_depends_on_container =
+ item_style.LogicalWidth().IsPercentOrCalc() ||
+ item_style.LogicalMinWidth().IsPercentOrCalc() ||
+ item_style.LogicalMaxWidth().IsPercentOrCalc();
+
+ const bool logical_height_depends_on_container =
+ item_style.LogicalHeight().IsPercentOrCalc() ||
+ item_style.LogicalMinHeight().IsPercentOrCalc() ||
+ item_style.LogicalMaxHeight().IsPercentOrCalc() ||
+ item_style.LogicalHeight().IsAuto();
+
+ // TODO(kschmi) - this should be using 'BlockLengthUnresolvable' and
+ // 'InlineLengthUnresolvable', however those are a too strict and don't
+ // end up laying out enough grid items.
+ const bool can_layout_block_axis =
+ space.AvailableSize().block_size != kIndefiniteSize ||
+ !logical_height_depends_on_container;
+ const bool can_layout_inline_axis =
+ space.AvailableSize().inline_size != kIndefiniteSize ||
+ !logical_width_depends_on_container;
+
+ return can_layout_inline_axis && can_layout_block_axis;
+ };
+
for (auto& grid_item : grid_items->item_data) {
if (!grid_item.IsBaselineSpecifiedForDirection(track_direction))
continue;
@@ -1861,30 +1875,33 @@
// baselines until layout has been performed. However, layout cannot
// be performed in certain scenarios. So force an additional pass in
// these cases and skip layout for now.
- if (!CanLayoutGridItem(grid_item, space, track_direction)) {
+ const auto& item_style = grid_item.node.Style();
+ if (!CanLayoutGridItem(item_style, space)) {
*needs_additional_pass = true;
continue;
}
- scoped_refptr<const NGLayoutResult> result = grid_item.node.Layout(space);
+ const auto result =
+ LayoutNodeForMeasure(grid_item.node, space, is_min_max_pass);
+
NGBoxFragment fragment(
- grid_item.node.Style().GetWritingDirection(),
+ item_style.GetWritingDirection(),
To<NGPhysicalBoxFragment>(result->PhysicalFragment()));
- const bool has_synthesized_baseline = HasSynthesizedBaseline(
- track_direction, fragment,
- ConstraintSpace().GetWritingDirection().GetWritingMode());
- grid_item.SetAlignmentFallback(track_direction, Style(),
- has_synthesized_baseline);
+ const auto& container_space = ConstraintSpace();
+ const auto container_writing_mode =
+ container_space.GetWritingDirection().GetWritingMode();
- const auto margins =
- ComputeMarginsFor(space, grid_item.node.Style(), ConstraintSpace());
- LayoutUnit margin = (track_direction == kForColumns) ? margins.inline_start
- : margins.block_start;
+ grid_item.SetAlignmentFallback(
+ track_direction, Style(),
+ HasSynthesizedBaseline(track_direction, fragment,
+ container_writing_mode));
+
+ const auto margins = ComputeMarginsFor(space, item_style, container_space);
LayoutUnit baseline =
- margin + GetLogicalBaseline(
- fragment, track_direction,
- ConstraintSpace().GetWritingDirection().GetWritingMode());
+ ((track_direction == kForColumns) ? margins.inline_start
+ : margins.block_start) +
+ GetLogicalBaseline(fragment, track_direction, container_writing_mode);
// TODO(kschmi): The IsReplaced() check here is a bit strange, but is
// necessary to pass some of the tests. Follow-up to see if there's
@@ -3341,7 +3358,7 @@
const auto& item_style = grid_item.node.Style();
const auto margins = ComputeMarginsFor(space, item_style, container_space);
- scoped_refptr<const NGLayoutResult> result = grid_item.node.Layout(space);
+ auto result = grid_item.node.Layout(space);
const auto& physical_fragment =
To<NGPhysicalBoxFragment>(result->PhysicalFragment());
NGBoxFragment logical_fragment(item_style.GetWritingDirection(),
diff --git a/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.h b/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.h
index a04eb47..b83a9057 100644
--- a/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.h
+++ b/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.h
@@ -20,8 +20,10 @@
using SetOffsetData = NGGridData::SetData;
-enum class AxisEdge : uint8_t { kStart, kCenter, kEnd, kBaseline };
-enum class ItemType : uint8_t { kInGridFlow, kOutOfFlow };
+enum class AxisEdge { kStart, kCenter, kEnd, kBaseline };
+enum class BaselineType { kMajor, kMinor };
+enum class ItemType { kInGridFlow, kOutOfFlow };
+enum class SizingConstraint { kLayout, kMinContent, kMaxContent };
// This enum corresponds to each step used to accommodate grid items across
// intrinsic tracks according to their min and max track sizing functions, as
@@ -35,11 +37,6 @@
kForFreeSpace,
};
-enum class BaselineType : uint8_t {
- kMajor,
- kMinor,
-};
-
struct GridItemIndices {
wtf_size_t begin = kNotFound;
wtf_size_t end = kNotFound;
@@ -333,8 +330,6 @@
private:
friend class NGGridLayoutAlgorithmTest;
- enum class SizingConstraint { kLayout, kMinContent, kMaxContent };
-
LayoutUnit ComputeIntrinsicBlockSizeIgnoringChildren() const;
// Returns the size that a grid item will distribute across the tracks with
@@ -378,16 +373,11 @@
const NGGridLayoutAlgorithmTrackCollection& track_collection,
GridItems* grid_items) const;
- // Returns 'true' if it's possible to layout a grid item.
- bool CanLayoutGridItem(
- const GridItemData& grid_item,
- const NGConstraintSpace& space,
- const GridTrackSizingDirection track_direction) const;
-
// Determines the major/minor alignment baselines for each row/column based
// on each item in |grid_items|, and stores the results in |grid_geometry|.
void CalculateAlignmentBaselines(
const GridTrackSizingDirection track_direction,
+ const bool is_min_max_pass,
GridGeometry* grid_geometry,
GridItems* grid_items,
bool* needs_additional_pass) const;
diff --git a/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm_test.cc b/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm_test.cc
index 3a308bac..ca4e737 100644
--- a/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm_test.cc
+++ b/third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm_test.cc
@@ -89,15 +89,15 @@
// Resolve inline size.
bool unused;
- algorithm.ComputeUsedTrackSizes(
- NGGridLayoutAlgorithm::SizingConstraint::kLayout, grid_geometry_,
- grid_properties, /* is_minmax_pass */ false, &column_track_collection_,
- &grid_items_, &unused);
+ algorithm.ComputeUsedTrackSizes(SizingConstraint::kLayout, grid_geometry_,
+ grid_properties, /* is_minmax_pass */ false,
+ &column_track_collection_, &grid_items_,
+ &unused);
// Resolve block size.
- algorithm.ComputeUsedTrackSizes(
- NGGridLayoutAlgorithm::SizingConstraint::kLayout, grid_geometry_,
- grid_properties, /* is_minmax_pass */ false, &row_track_collection_,
- &grid_items_, &unused);
+ algorithm.ComputeUsedTrackSizes(SizingConstraint::kLayout, grid_geometry_,
+ grid_properties, /* is_minmax_pass */ false,
+ &row_track_collection_, &grid_items_,
+ &unused);
}
NGGridLayoutAlgorithmTrackCollection& TrackCollection(
diff --git a/third_party/blink/web_tests/fast/css-grid-layout/crash-grid-area-dependent-baseline.html b/third_party/blink/web_tests/fast/css-grid-layout/crash-grid-area-dependent-baseline.html
new file mode 100644
index 0000000..302a2a3b
--- /dev/null
+++ b/third_party/blink/web_tests/fast/css-grid-layout/crash-grid-area-dependent-baseline.html
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<style>
+#target {
+ display: grid;
+ align-items: baseline;
+ writing-mode: vertical-rl;
+ padding-bottom: 50%;
+ line-height: 800px;
+}
+#target > div {
+ overflow: hidden;
+}
+</style>
+<div style="float: right; width: 20px; height: 20px;"></div>
+<div id="target">
+ <div style="width: -webkit-fill-available">fill-available</div>
+ <div style="width: fit-content">fit-content</div>
+</div>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script>
+ test(function() {
+ document.body.offsetTop;
+ }, "Test that computing a baseline dependent on its respective grid area's size doesn't crash.");
+</script>