Closed Bug 1298401 Opened 4 years ago Closed 4 years ago

RequestContentRepaint should check for scroll update type as well

Categories

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

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 - fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

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
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
Priority: -- → P1
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
Attachment #8785323 - Flags: review?(tnikkel) → review+
Pushed by kgupta@mozilla.com:
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 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
Attachment #8785323 - Flags: approval-mozilla-beta?
Attachment #8785323 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b07502b5c423
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
No longer blocks: 1298254
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+
Attachment #8785323 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[Tracking Requested - why for this release]: bad regression that affects 49, with a low-risk fix
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.
Attachment #8785323 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Un-track for 49 as this has been fixed already.
No longer blocks: 1298252
No longer blocks: 1276077
Duplicate of this bug: 1276077
You need to log in before you can comment on or make changes to this bug.