Skip to content

Commit

Permalink
[GridFragmentation] Store placement data on NGGridLayoutData
Browse files Browse the repository at this point in the history
During fragmentation, we query the `NGGridLayoutData` from the
`NGGridBreakTokenData`, which includes track data. However, grid
layout has a strong dependency on item placement in addition to track
data. This was causing a crash in fragmentation, when we query the
track data from `NGGridLayoutData` (off of `NGGridBreakTokenData`), but
not the `GridItemData` placement or range indices. When these don't
match up, a crash was happening in `GridItemData::ComputeSetIndices`
during printing.

This change adds several more placement-related vectors on
`NGGridLayoutData` off of `NGGridBreakTokenData` so that layout can
proceed without crashing during fragmentation.

(cherry picked from commit 1c88f35)

Bug: 1399336
Change-Id: I4a19afd4718089e27456285b2955ebd7094212b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4109928
Commit-Queue: Kurt Catti-Schmidt <kschmi@microsoft.com>
Reviewed-by: Alison Maher <almaher@microsoft.com>
Cr-Original-Commit-Position: refs/heads/main@{#1084104}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4126321
Commit-Queue: Ethan Jimenez <ethavar@microsoft.com>
Cr-Commit-Position: refs/branch-heads/5481@{#102}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
KurtCattiSchmidt authored and Chromium LUCI CQ committed Jan 3, 2023
1 parent 52d399a commit c5f7920
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_GRID_NG_GRID_BREAK_TOKEN_DATA_H_

#include "third_party/blink/renderer/core/layout/ng/grid/ng_grid_data.h"
#include "third_party/blink/renderer/core/layout/ng/grid/ng_grid_item.h"
#include "third_party/blink/renderer/core/layout/ng/ng_block_break_token_data.h"
#include "third_party/blink/renderer/core/layout/ng/ng_fragmentation_utils.h"
#include "third_party/blink/renderer/platform/geometry/layout_unit.h"
Expand Down Expand Up @@ -34,6 +35,9 @@ struct NGGridBreakTokenData final : NGBlockBreakTokenData {
const NGGridLayoutData& layout_data,
LayoutUnit intrinsic_block_size,
LayoutUnit consumed_grid_block_size,
Vector<GridItemIndices>&& column_range_indices,
Vector<GridItemIndices>&& row_range_indices,
Vector<GridArea>&& resolved_positions,
const Vector<GridItemPlacementData>& grid_items_placement_data,
const Vector<LayoutUnit>& row_offset_adjustments,
const Vector<EBreakBetween>& row_break_between,
Expand All @@ -42,6 +46,9 @@ struct NGGridBreakTokenData final : NGBlockBreakTokenData {
layout_data(layout_data),
intrinsic_block_size(intrinsic_block_size),
consumed_grid_block_size(consumed_grid_block_size),
column_range_indices(std::move(column_range_indices)),
row_range_indices(std::move(row_range_indices)),
resolved_positions(std::move(resolved_positions)),
grid_items_placement_data(grid_items_placement_data),
row_offset_adjustments(row_offset_adjustments),
row_break_between(row_break_between),
Expand All @@ -59,7 +66,9 @@ struct NGGridBreakTokenData final : NGBlockBreakTokenData {
// it isn't used for determining the final block-size of the fragment and
// won't include any block-end padding (this prevents saturation bugs).
LayoutUnit consumed_grid_block_size;

Vector<GridItemIndices> column_range_indices;
Vector<GridItemIndices> row_range_indices;
Vector<GridArea> resolved_positions;
Vector<GridItemPlacementData> grid_items_placement_data;
Vector<LayoutUnit> row_offset_adjustments;
Vector<EBreakBetween> row_break_between;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,30 @@ namespace {

void CacheGridItemsProperties(
const NGGridLayoutTrackCollection& track_collection,
GridItems* grid_items) {
GridItems* grid_items,
const Vector<GridItemIndices>* range_indices = nullptr,
const Vector<GridArea>* resolved_positions = nullptr) {
DCHECK(grid_items);

GridItemDataPtrVector grid_items_spanning_multiple_ranges;
const auto track_direction = track_collection.Direction();

for (auto& grid_item : *grid_items) {
const auto& range_indices = grid_item.RangeIndices(track_direction);
for (wtf_size_t index = 0; index < grid_items->Size(); ++index) {
auto& grid_item = grid_items->At(index);

// If positions range indices were provided, assign them before querying
// `RangeIndices` below.
if (resolved_positions && resolved_positions->size())
grid_item.resolved_position = resolved_positions->at(index);

if (range_indices && range_indices->size()) {
if (track_direction == kForColumns)
grid_item.column_range_indices = range_indices->at(index);
else
grid_item.row_range_indices = range_indices->at(index);
}

const auto& item_range_indices = grid_item.RangeIndices(track_direction);
auto& track_span_properties = (track_direction == kForColumns)
? grid_item.column_span_properties
: grid_item.row_span_properties;
Expand All @@ -113,9 +129,9 @@ void CacheGridItemsProperties(
// to do more work to cache its track span properties.
//
// TODO(layout-dev): Investigate applying this concept to spans > 1.
if (range_indices.begin == range_indices.end) {
if (item_range_indices.begin == item_range_indices.end) {
track_span_properties =
track_collection.RangeProperties(range_indices.begin);
track_collection.RangeProperties(item_range_indices.begin);
} else {
grid_items_spanning_multiple_ranges.emplace_back(&grid_item);
}
Expand Down Expand Up @@ -238,8 +254,16 @@ const NGLayoutResult* NGGridLayoutAlgorithm::LayoutInternal() {
intrinsic_block_size = grid_data->intrinsic_block_size;
layout_data = grid_data->layout_data;

CacheGridItemsProperties(*layout_data.Columns(), &grid_items);
CacheGridItemsProperties(*layout_data.Rows(), &grid_items);
// Update `grid_items` with resolved positions and range indices stored
// on the break token, as these are dependent on the `layout_data` above.
//
// TODO(kschmi): If these don't change between fragmentainers, we can store
// them (and Columns/Rows) on `NGGridBreakTokenData` and avoid recomputing.
CacheGridItemsProperties(*layout_data.Columns(), &grid_items,
&grid_data->column_range_indices,
&grid_data->resolved_positions);
CacheGridItemsProperties(*layout_data.Rows(), &grid_items,
&grid_data->row_range_indices);
} else {
ComputeGridGeometry(&grid_sizing_tree, &intrinsic_block_size);
}
Expand All @@ -251,6 +275,9 @@ const NGLayoutResult* NGGridLayoutAlgorithm::LayoutInternal() {
LayoutUnit consumed_grid_block_size;
Vector<GridItemPlacementData> grid_items_placement_data;
Vector<LayoutUnit> row_offset_adjustments;
Vector<GridItemIndices> column_range_indices;
Vector<GridItemIndices> row_range_indices;
Vector<GridArea> resolved_positions;
if (UNLIKELY(InvolvedInBlockFragmentation(container_builder_))) {
// Either retrieve all items offsets, or generate them using the
// non-fragmented |PlaceGridItems| pass.
Expand All @@ -264,6 +291,12 @@ const NGLayoutResult* NGGridLayoutAlgorithm::LayoutInternal() {
row_break_between = grid_data->row_break_between;
oof_children = grid_data->oof_children;
} else {
for (auto& grid_item : grid_items) {
column_range_indices.push_back(grid_item.column_range_indices);
row_range_indices.push_back(grid_item.row_range_indices);
resolved_positions.push_back(grid_item.resolved_position);
}

row_offset_adjustments =
Vector<LayoutUnit>(layout_data.Rows()->GetSetCount() + 1);
PlaceGridItems(grid_items, layout_data, &row_break_between,
Expand Down Expand Up @@ -344,8 +377,9 @@ const NGLayoutResult* NGGridLayoutAlgorithm::LayoutInternal() {
MakeGarbageCollected<NGGridBreakTokenData>(
container_builder_.GetBreakTokenData(), layout_data,
intrinsic_block_size, consumed_grid_block_size,
grid_items_placement_data, row_offset_adjustments,
row_break_between, oof_children));
std::move(column_range_indices), std::move(row_range_indices),
std::move(resolved_positions), grid_items_placement_data,
row_offset_adjustments, row_break_between, oof_children));
}

// Copy grid layout data for use in computed style and devtools.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<!DOCTYPE html>
<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1399336">
<body>
<div style="width: 2000px; height: 2000px;">
<div style="columns: 2; border: 1px solid black;">
<div style="height: 50px; width: 550px; float: right;">Text</div>
<div style="display: grid; grid-template-columns: repeat(auto-fill,250px);">
<div style="width: 250px;">
<div style="height: 550px;"></div>
</div>
<div style="width: 250px;">
<div style="height: 550px;"></div>
</div>
</div>
</div>
</div>
</body>

0 comments on commit c5f7920

Please sign in to comment.