Robustify against multiple scrollframes doing interleaved smooth scrolling
Categories
(Core :: Panning and Zooming, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox84 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
(Keywords: correctness, Whiteboard: [gfx-noted])
Attachments
(2 files)
See bug 1228407 comment 17 for backstory. The patch in that bug was intended to make APZ smooth scrolling better when there's a continual flow of incoming smooth-scroll requests from JS. However the patch leaves an edge case unfixed, where if the page has two separate scrollframes and they are getting smooth-scrolled by JS in an interleaved fashion, then we might run into badness. Filing this bug to take care of this in a follow-up.
|
Assignee | |
Updated•8 years ago
|
|
||
Updated•7 years ago
|
|
|
Assignee | |
Comment 1•4 years ago
|
||
This is a little puzzling to me. In a process with a single scrollframe, mScrollGeneration is always equal to sScrollGenerationCounter, because mScrollGeneration is only ever assigned to by statements of the form mScrollGeneration = ++sScrollGenerationCounter. So we can say that mScrollGeneration != sScrollGenerationCounter if and only if there is another scrollframe in the process, and that scrollframe got a new scroll request. And that's precisely the case that we want to ignore in this bug, which means we should be able to accomplish it by simply removing that clause from the if condition. The fact that mApzSmoothScrollDestination gets reset to Nothing() via other scroll methods should be sufficient to guarantee correctness.
|
|
Assignee | |
Comment 2•4 years ago
|
||
Not a strict blocker, but getting rid of the use site of sScrollGenerationCounter makes things cleaner.
|
|
Assignee | |
Comment 3•4 years ago
|
||
I verified my intuition in comment 1 with tests and a patch. https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=aed298abe93fbfbef226e9a5fd2ec31f37eacc23
|
|
Assignee | |
Comment 4•4 years ago
|
||
Retro-test to ensure we don't regress the scenario from bug 1228407.
|
|
Assignee | |
Comment 5•4 years ago
|
||
In the case where there are two scrollframes that get scrolled in an
interleaved manner, the scroll generation counter continually advances
and blocks APZ from updating the main-thread scroll position. This
fixes that by removing the scroll generation counter check from the
relevant condition. The clause is entirely redundant when there is only
a single scrollframe and only causes problems when there are multiple
scrollframes.
Depends on D97033
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/05c9cf63ecb0 Add a test for bug 1228407. r=botond https://hg.mozilla.org/integration/autoland/rev/f970b5bdfdbe Allow scroll position to advance when spamming multiple scrollframes in an interleaved manner. r=botond
|
||
Comment 7•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/05c9cf63ecb0
https://hg.mozilla.org/mozilla-central/rev/f970b5bdfdbe
Description
•