1298401 - RequestContentRepaint should check for scroll update type as well

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P1)

Description

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

Since bug 1286179 landed, a number of intermittent failures have spiked in volume. I was able to reproduce bug 1276077 locally with logging and I think it happens because of the following sequence of events:

1) APZ does a scroll animation, so mFrameMetrics's scroll position is updated
2) APZ gets a NotifyLayersUpdated and triggers a repaint request with eNone update type
3a) The repaint request goes to the main thread, but skips updating the scroll position
3b) The scroll animation ends, triggers a repaint request with eUserAction update type. However this is ignored at [1] because the scroll position is the same as the last repaint request

This results in the main thread getting a stale scroll offset.

[1] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/gfx/layers/apz/src/AsyncPanZoomController.cpp#2870

Comment 1

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

Attachment: Bug 1298401 - Ensure that eNone scroll update types from the APZC don't prevent eUserAction scroll updates from getting sent.

Review commit: https://reviewboard.mozilla.org/r/74570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74570/

Comment 2

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

Flagging for review sooner rather than later since this should be uplifted to aurora and beta. Try push in progress at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b19c65726b

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8785323 [details]
Bug 1298401 - Ensure that eNone scroll update types from the APZC don't prevent eUserAction scroll updates from getting sent.

https://reviewboard.mozilla.org/r/74570/#review72440

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b07502b5c423
Ensure that eNone scroll update types from the APZC don't prevent eUserAction scroll updates from getting sent. r=tnikkel

Comment 5

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

Comment on attachment 8785323 [details]
Bug 1298401 - Ensure that eNone scroll update types from the APZC don't prevent eUserAction scroll updates from getting sent.

Approval Request Comment
[Feature/regressing bug #]: bug 1286179
[User impact if declined]: In some cases the main thread scroll position might be left at a different value than the APZ. This can result in the visual scroll position jumping unexpectedly. Content JS might also read the wrong scroll position, which could result in it mispositioning content on the screen.
[Describe test coverage new/current, TreeHerder]: many tests exercise this code, and their intermittent failure rate spiked significantly with bug 1286179. I expect them to return to their normal values with this fix.
[Risks and why]: Very low risk, this should be a very safe patch from a correctness point of view. It might result in a few extra repaint requests getting sent but that's not going to be noticeable, even from a perf standpoint.
[String/UUID change made/needed]: none

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b07502b5c423

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8785323 [details]
Bug 1298401 - Ensure that eNone scroll update types from the APZC don't prevent eUserAction scroll updates from getting sent.

Fixes a regression in scroll position that can be quite annoying if left unfixed, Aurora50+

Comment 8

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6d69919d05a

Comment 9

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

[Tracking Requested - why for this release]: bad regression that affects 49, with a low-risk fix

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8785323 [details]
Bug 1298401 - Ensure that eNone scroll update types from the APZC don't prevent eUserAction scroll updates from getting sent.

Regression in 49, let's uplift this now for the 49 beta 9 build tomorrow.

Comment 11

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

https://hg.mozilla.org/releases/mozilla-beta/rev/9f202fc87b21

Comment 12

[Gerry Chang [:gchang]]

Un-track for 49 as this has been fixed already.