Commit e3add48b authored by Aleks Totic's avatar Aleks Totic Committed by Commit Bot

[LayoutNG] Fix access to deleted OOF descendant

This patch fixes the reported crash. The reproducible case is
https://ajuntament.barcelona.cat/widgets/socials/bcn/index.html?lang=es&numRows=1&header&menu&boxColor=aab0b0&buttonColor=aab0b0&buttonRadius=50&fontColor=ffffff&id=3c58a16711f992fee70edcf13ef72da3

Unfortunately, I have been unable to create a minimal reproducible
case that causes a crash. The added test case exercises new
code path, just without a crash.

The bug observed at the crashing URL was:

LayoutResult::OOFPositionedDescendants had an invalid descendant
that was already removed from the tree. What made this failure extra
annoying is that memory occupied by invalid descendant was reused
for a different LayoutObject.

Removal of the descendant caused LayoutObject::MarkContainerChainForLayout
to be called, but the container chain was not fully traversed because
ObjectIsRelayoutBoundary returned true and aborted the traversal.

My understanding is as following. In 2015, leviw introduced an
invalidation optimization. The optimization happens when
child needs layout, but can guarantee that it will not affect
layout of its parent.

In this case, child stops marking parents for relayout, and is
instead put on LayoutSubtreeRootList, which gets traversed
for layout directly. Method that does this is
LocalFrameView::ScheduleRelayoutOfSubtree

The method that decides whether child affects parents layout is:
ObjectIsRelayoutBoundary(). I've fixed this method for NG, so
that cached layout result with oof descendants causes full
container chain traversal.

Bug: 933054
Change-Id: Ie3daab53937297f1cea75b0974c3fce353c86200
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1500610
Commit-Queue: Aleks Totic <atotic@chromium.org>
Reviewed-by: default avatarMorten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637821}
parent 2f034a6e
...@@ -853,11 +853,21 @@ static inline bool ObjectIsRelayoutBoundary(const LayoutObject* object) { ...@@ -853,11 +853,21 @@ static inline bool ObjectIsRelayoutBoundary(const LayoutObject* object) {
if (object->IsLayoutScrollbarPart()) if (object->IsLayoutScrollbarPart())
return false; return false;
// In general we can't relayout a flex item independently of its container; if (const LayoutBox* layout_box = ToLayoutBoxOrNull(object)) {
// not only is the result incorrect due to the override size that's set, it // In general we can't relayout a flex item independently of its container;
// also messes with the cached main size on the flexbox. // not only is the result incorrect due to the override size that's set, it
if (object->IsBox() && ToLayoutBox(object)->IsFlexItemIncludingNG()) // also messes with the cached main size on the flexbox.
return false; if (layout_box->IsFlexItemIncludingNG())
return false;
// In LayoutNG, if box has any OOF descendants, they are propagated to
// parent. Therefore, we must mark parent chain for layout.
if (layout_box->GetCachedLayoutResult() &&
layout_box->GetCachedLayoutResult()
->OutOfFlowPositionedDescendants()
.size() > 0)
return false;
}
// Inside multicol it's generally problematic to allow relayout roots. The // Inside multicol it's generally problematic to allow relayout roots. The
// multicol container itself may be scheduled for relayout as well (due to // multicol container itself may be scheduled for relayout as well (due to
......
...@@ -149,6 +149,7 @@ crbug.com/591099 external/wpt/css/css-masking/clip-path/clip-path-ellipse-007.ht ...@@ -149,6 +149,7 @@ crbug.com/591099 external/wpt/css/css-masking/clip-path/clip-path-ellipse-007.ht
crbug.com/591099 external/wpt/css/css-masking/clip-path/clip-path-polygon-010.html [ Failure ] crbug.com/591099 external/wpt/css/css-masking/clip-path/clip-path-polygon-010.html [ Failure ]
crbug.com/591099 external/wpt/css/css-multicol/multicol-span-all-005.html [ Failure ] crbug.com/591099 external/wpt/css/css-multicol/multicol-span-all-005.html [ Failure ]
crbug.com/591099 external/wpt/css/css-multicol/multicol-span-all-list-item-002.html [ Pass ] crbug.com/591099 external/wpt/css/css-multicol/multicol-span-all-list-item-002.html [ Pass ]
crbug.com/933054 external/wpt/css/css-position/position-absolute-container-dynamic-002.html [ Pass ]
crbug.com/591099 external/wpt/css/css-position/position-relative-table-tbody-top-absolute-child.html [ Failure ] crbug.com/591099 external/wpt/css/css-position/position-relative-table-tbody-top-absolute-child.html [ Failure ]
crbug.com/591099 external/wpt/css/css-position/position-relative-table-tr-left-absolute-child.html [ Failure ] crbug.com/591099 external/wpt/css/css-position/position-relative-table-tr-left-absolute-child.html [ Failure ]
crbug.com/591099 external/wpt/css/css-position/static-position/htb-ltr-rtl.tentative.html [ Pass ] crbug.com/591099 external/wpt/css/css-position/static-position/htb-ltr-rtl.tentative.html [ Pass ]
......
...@@ -465,6 +465,7 @@ crbug.com/711807 external/wpt/css/CSS2/normal-flow/replaced-intrinsic-001.xht [ ...@@ -465,6 +465,7 @@ crbug.com/711807 external/wpt/css/CSS2/normal-flow/replaced-intrinsic-001.xht [
crbug.com/711807 external/wpt/css/CSS2/normal-flow/replaced-intrinsic-002.xht [ Failure ] crbug.com/711807 external/wpt/css/CSS2/normal-flow/replaced-intrinsic-002.xht [ Failure ]
#### external/wpt/css/css-position #### external/wpt/css/css-position
crbug.com/933054 external/wpt/css/css-position/position-absolute-container-dynamic-002.html [ Failure ]
crbug.com/752022 external/wpt/css/css-position/position-sticky-offset-overflow.html [ Failure ] crbug.com/752022 external/wpt/css/css-position/position-sticky-offset-overflow.html [ Failure ]
crbug.com/702927 external/wpt/css/css-position/position-sticky-table-tr-top.html [ Failure ] crbug.com/702927 external/wpt/css/css-position/position-sticky-table-tr-top.html [ Failure ]
crbug.com/702927 external/wpt/css/css-position/position-sticky-table-tr-bottom.html [ Failure ] crbug.com/702927 external/wpt/css/css-position/position-sticky-table-tr-bottom.html [ Failure ]
......
<!DOCTYPE html>
<title>CSS Position Absolute: dynamic changes to containing block height</title>
<link rel="author" href="mailto:atotic@google.com">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=933054">
<meta name="assert" content="Chrome regression: abspos descendant responds to containing block size change through line items">
<style>
#container {
position: relative;
}
#intermediate {
overflow: hidden;
width:200px;
height:200px;
background:red;
}
#block {
height:200px;
background:green;
}
#target {
position: absolute;
width: 200px;
height: 100px;
background:green;
}
</style>
<!-- Test for crbug.com/933054
Relayout optimizations cause OOF descendant not to be
repositioned
-->
<div id="container">
<div id="intermediate">
<div id="block"></div>
<div id="target"></div>
</div>
</div>
<script>
document.body.offsetTop;
test(() => {
document.getElementById("block").style.height = "100px";
assert_equals(document.querySelector("#target").offsetTop, 100);
}, '#target static position responded to parent relayout');
</script>
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment