1664596 - Investigate potential correctness problem with main-thread tracking of APZ animations

Status: RESOLVED WORKSFORME

(Core :: Layout: Scrolling and Overflow, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

Spinoff from https://phabricator.services.mozilla.com/D88751#inline-510402. The ScrollFrameHelper::IsAnimating function checks some state to see if APZ might be animating a scroll. However this may return false if the main thread has received a smooth scroll request, followed by an instant relative scroll update. In this case the main-thread detects "no animation" because the instant relative scroll update clears the state used by IsAnimating, but on the APZ side the instant relative scroll update may end up just updating the destination of the smooth scroll animation, and leave the animation in-progress.

If the IsAnimating function is queried between these updates going into the mScrollUpdates array and before APZ does a repaint request to set the mApzAnimationInProgress flag, the value returned may be incorrect.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Note: the IsAnimating function referred to in comment 0 got renamed to IsScrollAnimating, so that's the name I use below.

I dug into this a bit and tried to come up with a test case that exposed a problem. The main thread scroll scroll request + instant relative scroll update can be triggered by e.g.:

    SpecialPowers.doCommand(window, "cmd_scrollLineDown");  // scroll request to trigger SmoothScrollAnimation
    scroller.scrollBy(0, 50);  // instant relative update

Note that it's important to trigger a SmoothScrollAnimation on the APZ side rather than a SmoothScrollMSDAnimation, because the MSD animation gets cancelled here by the instant relative update, whereas the non-MSD animation does not.

In this scenario, the IsScrollAnimating check here does indeed return false, but the subsequent line also checks the LastScrollOrigin on the main thread, and that clause returns true. So the net result is that APZCCallbackHelper::IsScrollInProgress still returns true and everything unfolds as it should.

There are a few other callsites of IsScrollAnimating, but again I couldn't really come up with a scenario where I could trigger a bug. At this callsite again the setup required to enter the codepath with IsScrollAnimating returning false means that other state (the last origin and mDestination) is also set up such that no bug manifests. The other two callsites I spent some (not much) time thinking about it but again couldn't come up with a plausible scenario where a bug might manifest.

Some of the code certainly seems brittle and could be robustified somehow, but I don't want to introduce regressions that I may not be around to deal with. So for now I'm inclined to close this bug as WFM unless we have a scenario that demonstrably produces a bug.