Skip to content

GitLab

T
tangled
Commit 2f1db462 authored by David Bokan's avatar David Bokan Committed by Commit Bot
Browse files
Options

[BGPT] Ensure cc::Layer cleans up ElementId mapping 

This bug occurs because of an issue in ordering of cc::Layer
destruction. LayerTreeHost maintains a mapping of ElementId to
cc::Layer. When scroll deltas are committed from the compositor thread
to the main thread, the deltas are associated with an ElementId. LTH
uses the mapping to determine the layer onto which the delta is applied.

The problem here occurs when a scroller loses its compositing layer,
then becomes composited again within the same lifecycle frame. The event
sequence:

1) Scroller becomes decomposited due to style change, GraphicsLayer is
destructed but the root cc::Layer still has a pointer to the child layer
so we don't yet destroy the associated cc::Layer
2) Scroller becomes composited again due to another style change.
3) During compositing portion of lifecycle we'll create a new
GraphicsLayer.
4) In PaintArtifactCompositor::Update, we create a new cc::Layer which
causes us to initialize the ElementId mapping in LTH when we call
Layer::SetLayerTreeHost. This clobbers the ElementId->cc::Layer mapping
for the layer removed in step 1.
5) At the end of PAC::Update, we set the new layer list on the root
which removes pointer to the layer removed in step 1. This causes the
layer to be deleted which calls cc::Layer::SetLayerTreeHost(nullptr)
causing the layer to remove its ElementId mapping. However, the mapping
for the ElementId now points to the new layer created in step 4.
6) Compositor scrolls against the ElementId now don't commit any delta
because the mapping doesn't contain an entry for ElementId.

This CL fixes the issue by causing the cc::Layer to remove itself from
the layer list when the GraphicsLayer is deleted. This will cause the
layer to be deleted and unregister itself at that time.

Bug: 979002
Change-Id: I76b195982cc09d9ed208e6f96271b7539898da4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1690036Reviewed-by: default avatarPhilip Rogers <pdr@chromium.org>
Reviewed-by: default avatarXianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Auto-Submit: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675740}
parent c0f498fc
Hide whitespace changes
Inline Side-by-side
Showing with 95 additions and 0 deletions
third_party/blink/renderer/platform/graphics/graphics_layer.cc
View file @ 2f1db462
...@@ -114,6 +114,12 @@ GraphicsLayer::~GraphicsLayer() { ...@@ -114,6 +114,12 @@ GraphicsLayer::~GraphicsLayer() {
RemoveAllChildren(); RemoveAllChildren();
RemoveFromParent(); RemoveFromParent();
DCHECK(!parent_); DCHECK(!parent_);
// This ensures we clean-up the ElementId to cc::Layer mapping in
// LayerTreeHost before a new layer with the same ElementId is added.
// Regression from BGPT: https://crbug.com/979002
if (RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled())
SetElementId(CompositorElementId());
} }
IntRect GraphicsLayer::VisualRect() const { IntRect GraphicsLayer::VisualRect() const {
......
third_party/blink/web_tests/FlagExpectations/enable-blink-features=CompositeAfterPaint
View file @ 2f1db462
...@@ -551,3 +551,7 @@ crbug.com/966981 virtual/threaded/fast/scrolling/events/scrollend-event-fired-to ...@@ -551,3 +551,7 @@ crbug.com/966981 virtual/threaded/fast/scrolling/events/scrollend-event-fired-to
crbug.com/966981 virtual/threaded/fast/scrolling/events/scrollend-event-fired-to-scrolled-element.html [ Timeout ] crbug.com/966981 virtual/threaded/fast/scrolling/events/scrollend-event-fired-to-scrolled-element.html [ Timeout ]
crbug.com/966981 virtual/threaded/fast/scrolling/events/scrollend-event-fired-to-window.html [ Timeout ] crbug.com/966981 virtual/threaded/fast/scrolling/events/scrollend-event-fired-to-window.html [ Timeout ]
crbug.com/966981 virtual/threaded/synthetic_gestures/animated-wheel-tiny-delta.html [ Failure ] crbug.com/966981 virtual/threaded/synthetic_gestures/animated-wheel-tiny-delta.html [ Failure ]
# Added to land BGPT-fix
crbug.com/979002 virtual/fractional_scrolling_threaded/fast/scrolling/uncomposite-and-composite-scroll.html [ Failure ]
crbug.com/979002 virtual/threaded/fast/scrolling/uncomposite-and-composite-scroll.html [ Failure ]
third_party/blink/web_tests/fast/scrolling/uncomposite-and-composite-scroll.html 0 → 100644
View file @ 2f1db462
<!DOCTYPE html>
<script src='../../resources/testharness.js'></script>
<script src='../../resources/testharnessreport.js'></script>
<script src='../../resources/gesture-util.js'></script>
<style type="text/css">
#outer {
background: #fff;
position: absolute
}
#inner {
max-height: 300px;
max-width: 116px;
overflow-y: auto;
will-change: transform;
}
/* Custom scrollbar so they're updated on the main thread only */
::-webkit-scrollbar {
height: 16px;
width: 16px
}
::-webkit-scrollbar-thumb {
background-color: grey;
}
/* An interesting background so we can tell we're scrolling */
#spacer {
height: 3000px;
width: 100px;
background-color: #FFF;
background-size: 50px 50px;
background-position: 0 0, 25px 25px;
background-image: linear-gradient(45deg, black 25%, transparent 25%, transparent 75%, black 75%, black),
linear-gradient(45deg, black 25%, transparent 25%, transparent 75%, black 75%, black);
}
</style>
<div id="outer">
<div id="inner">
<div id="spacer"></div>
</div>
<p>
Scroll the above box with wheel or touch. If the scrollbar moves with the
scroll the test passes.
</p>
</div>
<script>
addEventListener('load', async () => {
let scroller = document.getElementById('inner');
// Add style that will cause the PaintLayer and associated compositing
// layers to get deleted.
scroller.style.overflowY = 'visible';
scroller.style.willChange = 'auto';
// Force a style and layout recalc.
scroller.offsetHeight;
// Remove the style so that a new PaintLayer and compositing layers are
// re-added.
scroller.style.willChange = '';
scroller.style.overflowY = '';
await waitForCompositorCommit();
promise_test(async () => {
const delta = 1000;
const location = { x: 50, y: 50 };
await smoothScroll(delta,
location.x,
location.y,
GestureSourceType.TOUCH_INPUT,
'down',
SPEED_INSTANT);
// Will throw exception if window.scrollY not greater than 0.
assert_greater_than(scroller.scrollTop, 0,
"Scroller should be scrolled.");
}, 'Tests that composited scrolling works after de-and-re-compositing scroller.');
});
</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