New issue
Advanced search Search tips

Issue 820839 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Scroll clamping during layout can fail to invalidate compositing

Project Member Reported by pdr@chromium.org, Mar 11 2018

Issue description

Chrome Version: 67.0.3367.0/Canary
OS: Android

What steps will reproduce the problem?
(1) Visit https://www.bogleheads.org/forum/viewtopic.php?f=10&t=240012&p=3753852#p3753852
(2) Scroll all the way to the bottom
(3) Scroll up a little and notice content disappears.

Steve and David, I think this is likely related to one of your recent patches, but I don't know which one.
 
scrollbug.png
196 KB View Download
Cc: skobes@chromium.org pnangunoori@chromium.org
Labels: hasbisect-per-revision Target-67 RegressedIn-67 Triaged-Mobile M-67 Needs-triage-Mobile FoundIn-67
Owner: chrishtr@chromium.org
Status: Assigned (was: Untriaged)
Tested the issue in Android and able to reproduce the issue. 

Steps Followed:
1. Launch Chrome.
2. Navigate to any URL: https://www.bogleheads.org/forum/viewtopic.php?f=10&t=240012&p=3753852#p3753852
3. Scroll the page up and down a little bit.
4. Observed that an empty space is displayed in the bottom of the page.

Some time the mentioned behavior is seen after scrolling up and down for couple of times.

Chrome versions tested:
67.0.3365.0, 67.0.3368.0(Canary)

OS:
Android 8.1.0, Android 7.0.0

Android Devices:
Pixel, Samsung J7

Using the per-revision bisect providing the bisect results,
Good Build - 67.0.3360.0 (540723)
Bad Build - 67.0.3361.0 (540766)

You are looking for a change made after 540745(GOOD), but before 540746(BAD).

CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.
https://chromium.googlesource.com/chromium/src/+/9732521599b291a0703830c0d9d94c381916b7d8

From the CL above, assigning the issue to the owner concerned.

@chrishtr:  Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to owner concerned.

Please navigate to below link for log's and video--
go/chrome-androidlogs/820839

Note: 
1. This issue is not observed in Desktop.
2. Similar issue is also seen in FireFox - Blank space is not displayed but when tried to scroll up and down scrolling is not done till bottom of the screen and seems to be truncated. However, upon scrolling down once again complete content gets loaded. Attached screen cast for FireFox as well.

Thanks!

Comment 2 by bokan@chromium.org, Mar 12 2018

Blocking: 417782
Cc: -bokan@chromium.org chrishtr@chromium.org
Labels: -Pri-2 Pri-1
Owner: bokan@chromium.org
I'm able to repro in 67.0.3365.0 on a Nexus 6. Turning off root-layer-scrolls with --disable-features=RootLayerScrolling fixes the issue so it's related to RLS. I'll investigate...

Comment 3 by bokan@chromium.org, Mar 12 2018

Blocking: -417782
Missed that this doesn't repro in 66 branch (66.0.3359.14) so this is a recent, unmerged patch.

Comment 4 by bokan@chromium.org, Mar 12 2018

Cc: -chrishtr@chromium.org bokan@chromium.org
Owner: chrishtr@chromium.org
Eh, I should really read the existing messages more closely. It is indeed the patch in #1 and Chris is better suited to look into it than me.
Confirmed that reverting my patch fixes this issue.
I think the bug is that the scroll offset due to the initial scroll due to a
deep link with <a> tag gets things messed up.

Comment 7 by pdr@chromium.org, Mar 12 2018

I think there may be a more fundamental bug with scroll offsets after navigation. Here's a repro for a scroll bug on Android:

1) Go to https://www.bogleheads.org
2) Click on a post that has a few comments (so you can scroll).
3) Scroll halfway down the page.
4) Press back, you should now be on https://www.bogleheads.org again.
5) Use the dots menu and navigate forward, you should now be scrolled halfway down a post.
6) Scroll up and down slowly, notice the scroll offset jumps to 0,0.

Comment 8 by pdr@chromium.org, Mar 12 2018

I've split off comment #7 into https://crbug.com/821191 and asked for a bisect.
Forcing off URL bar hiding causes the original bug here not to
reproduce.
What happens is that after scroll all the way to the bottom of the webpage,
another attempt to scroll up will for some reason result in PLSA::UpdateScrollDimensions getting called, which then adds 25px of height
for some reason. Then the next attempt to scroll will scroll into these 25px.
I was able to fix the bug by restricting the fast path to a ScrolType of
kCompositorScroll.
Labels: -Needs-Bisect
Removing 'Needs-Bisect' label.
The root cause is that CompositedLayerMapping::UpdateScrollingLayerGeometry
decides whether to update scroll offset based on whether it appears that
the parameters for the scroller have changed (overflow size, scroll offset in
PaintLayerScrollableArea, scroller size).

But those could have changed to another value and then changed back to the
current ones between compositing updates, via JS like this:

changeLayout();
element.offsetWidth;  // Forces style and layout (but not compositing), including
                      // scroll anchoring adjustment. Scroll anchoring adjustment can
                      // change scroll offset in PLSA.
changeLayoutBackToPrevious();
element.offsetWidth;  // Again forces style and layout, setting things back, but not necessarily
                      // adjusting scroll offset back (*)

(*) Another root cause could be that should be setting scroll offset correctly and it's not sometimes.
If the scroll offset changes during the first element.offsetWidth, and doesn't change back during the second element.offsetWidth, why wouldn't CompositedLayerMapping see a different offset from the previous frame?
Because PLSA is clamping the scroll back to the offset it was the last
time CLM was run, upon PLSA resize. Digging now into the exact reason and what
changed.
Indeed, moving UpdateCompositingLayersAfterScroll() out of the
if (!frame_view->IsInPerformLayout()) in PaintLayerScrollableArea::UpdateScrollOffset
appears to fix the bug, because UpdateScrollOffset is indeed called
during clamping, but in this situation the clamping is during layout.
Summary: Scroll clamping during layout can fail to invalidate compositing (was: Url bar hiding clip bug)
Project Member

Comment 18 by bugdroid1@chromium.org, Mar 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d

commit bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Thu Mar 15 02:24:30 2018

[RLS] Always update compositing layer offset and related dirty bits when scroll offset changes.

Previously, it did not update update them during layout, which could result in failure to update
composited scroll offset after situations like scroll clamping during synchronous
forced layouts.

Bug:  820839 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I129e7db4c5f4bc7b862b38de84ff174e7950fd5c
Reviewed-on: https://chromium-review.googlesource.com/963517
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543287}
[add] https://crrev.com/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d/third_party/WebKit/LayoutTests/compositing/scroll-update-with-clamp-expected.html
[add] https://crrev.com/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d/third_party/WebKit/LayoutTests/compositing/scroll-update-with-clamp.html
[modify] https://crrev.com/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp

Status: Fixed (was: Assigned)

Comment 20 by bokan@chromium.org, Mar 19 2018

Issue 821699 has been merged into this issue.

Sign in to add a comment