Hi David, could you triage this?

This was added in https://codereview.chromium.org/1648293003/ in support of  issue 581875 . This is trying to fix discontinuities in smooth scrolls where we start an animation on the main thread but, mid-animation, the scroller is made to be able to scroll on the compositor (e.g. JS removes position: fixed). At that point, it seems that a new scroll would occur on the compositor thread while the animation is playing on the main thread.

From the bug, it looks like this was an issue with flinging. Flings have since moved to the browser side, I wonder if this would still be broken today. I'll see if there's still issues if we don't set the MTSR

Fling and smooth scroll are totally separate, so I don't think moving fling to browser affects this.

Maybe a bit of a long shot since it's been 3 years but do you remember what the issue was? In https://bugs.chromium.org/p/chromium/issues/detail?id=581875#c8 Yash noted the issue was:

"Erratic jump when flinging as position:fixed is removed."

I'm confused since this seems like a really improbable case: an animation is running as a result of keyboard/wheel scrolling but a fling can only be started by a touchscreen or high-precision touchpad. Presumably users weren't performing scrolling on two separate devices at the same time?

So my assumption is that this was the fling animation that used to be animated in the renderer but I don't understand why a main thread scrolling reason would help in that case. Or is the comment misleading and fling didn't have anything to do with it?

In https://chromium-review.googlesource.com/c/chromium/src/+/1401277 I've added a commented-out TODO that we do not call cc::Layer::AddMainThreadScrollingReasons in layer-lists mode. When this bug is fixed, we should uncomment that DCHECK.

I think the reference to flinging is incorrect, the issue was just wheel scrolling on a page with a JS sticky implementation (some element becomes position:fixed below a certain scroll offset).  We had problems in both directions:

(1) If you start scrolling in the unfixed state, you have an active compositor-thread scroll animation.  Then we cross the sticky threshold and have to move that animation to the main thread.  This is super complicated plumbing around NotifyAnimationTakeover / AnimationEvent::TAKEOVER.

(2) If you start scrolling in the fixed state you have animation on MAIN, but when we cross the sticky threshold we remove kHasNonLayerViewportConstrainedObjects from the main thread scrolling reasons.  In this case, rather than transferring the animation to cc we just keep it on main and add a "fake" main thread scrolling reason so that the new scroll inputs continue to route into it.  This reason is kHandlingScrollFromMainThread.  If we didn't add it, the new scroll inputs would start a separate animation on the compositor thread that fights with the one on main.

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

commit 0ea3826e22bdda4e42ba818869cf643f271669d2
Author: Philip Rogers <pdr@chromium.org>
Date: Thu Jan 10 02:35:09 2019

[BlinkGenPropertyTrees] Do not set main thread reasons for overlays

This patch stops calling cc::Layer::AddMainThreadScrollingReasons for
overlays when using layer lists (aka BlinkGenPropertyTrees) because
these reasons need to be set on ScrollNodes. Because the frame overlay
uses the root scroll node, no main thread reason is actually needed
because it will not scroll.

This is the penultimate patch towards removing layer-list-mode callers
of Layer::AddMainThreadScrollingReasons. https://crbug.com/919969 has
been filed to fix the one remaining issue and a commented-out TODO has
been added in this patch to DCHECK that
Layer::AddMainThreadScrollingReasons is not called in layer lists mode.

Bug:  915372 , 919969
Change-Id: I31ffbb452b843fe7f498e23b1cffe429eb682a0c
Reviewed-on: https://chromium-review.googlesource.com/c/1401277
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621430}
[modify] https://crrev.com/0ea3826e22bdda4e42ba818869cf643f271669d2/cc/layers/layer.cc
[modify] https://crrev.com/0ea3826e22bdda4e42ba818869cf643f271669d2/third_party/blink/renderer/core/frame/frame_overlay.cc

 Issue 649126  has been merged into this issue.

BTW chrishtr was thinking of promoting fixed elements on low DPI (issue 921242).  If that lands, this becomes much less important.

I've come up with a repro: https://output.jsbin.com/xoruxur/4/quiet

I had to slow down the animations by a large factor to see it easily by changing kInverseDeltaMaxDuration to 120.0 in scroll_offset_animation_curve.cc. I can't reproduce any real issues without doing this.

The steps:

- Make sure you're using low dpi or --force-device-scale-factor=1
- Enable BGPT
- Scroll down so that the red box becomes position: fixed which is at 300px. A 400px window.scrollY is good
- Start scrolling up with a mouse wheel. Tick the wheel ~2/second. You don't want the animation to stop but you also don't want the wheels to be interpreted as a single gesture. A wheel event < 500ms after the last one will send an GestureScrollUpdate rather than a GestureScrollBegin so it'll be routed to the animating thread. Moving the cursor between wheel ticks will also ensure we always send a GSB
- When the box starts to move (becomes non-fixed), quickly start to scroll in the opposite direction

You'll see the page continue to scroll up, then a sudden jump down.

In addition to #10, this is also mitigated by latching as well. The max animation time for wheels is 200ms so wheels within that duration will only produce a ScrollBegin if the user also moves the cursor. When we're latched, the GSU will be handled on the correct thread already.

We should still fix this but this is definitely not a blocker. 

That's interesting that the gesture code mitigates this by routing to the right thread.   Issue 581875  predates wheel gestures.

It would be nice to synchronize gesture recognition with scroll animations.  Today they have two independent concepts of "is a scroll in progress", which isn't ideal.

Do you mean that we would detect in the recognizer that an animation is still playing and prevent breaking the latch (i.e. sending a GSU)? That would be complicated since recognition happens all the way up in the browser and we'd need to hit test to determine which element will be scrolled.

Or the other way around - use gesture end as the signal to tear down the animation, so gesture update always uses the UpdateTarget path (and gesture begin never does).

I think gesture timeout is currently longer than max animation duration, which may be why you had to tweak kInverseDeltaMaxDuration to repro.