Simon Lydell Reporter
	Description • 9 years ago

1. Open the attched test.html in a clean profile in Firefox with multi-process (e10s) enabled. The test page allows to scroll the page by using for instance `document.documentElement.scrollBy({top: 60, behavior: 'smooth'})` on each 'keydown' event for the 'k' key.
2. Hold down the 'k' key. This scrolls the page downwards smoothly as expected, but after a short while it stops, and then scrolls down in occasional outbursts. (The same thing happens when using the other scrolling keys mentioned in the test page.)
3. Turn off multi-process (e10s).
4. Hold the 'k' key on the test page again. Now the page scrolls downwards smoothly as expected, and keeps doing so in a constant speed for as long as 'k' is held.

I’d expect `element.scroll({behavior: 'smooth'})` to have the same behavior when called repeatedly, before the previous call has finished scrolling, regardless of whether multi-process is enabled or not. Preferably, the current non-multi-process behavior.

I’m a developer of the [VimFx][1] add-on, which uses the technique shown in the test page to implement the scrolling keyboard shortcuts of the add-on. While that is my primary use case, I think it is important that DOM methods that can be used by web page content (as demonstrated in the test page) behave the same regardless of whether multi-process is enabled or not.

If multi-process-by-default would be shipped to everyone tomorrow, I’d have to replace `element.scroll({behavior: 'smooth'})` with a custom implementation, which is a shame since it works so well in non-multi-process, and would replace one line of code with, say, a hundred or so for no reason.

[1]: https://github.com/akhodakivskiy/VimFx

	Mike Conley (:mconley) (:⚙️)
	Comment 1 • 9 years ago

Hey Simon,

Do you still see the bug with APZ disabled? You can disable APZ by setting layers.async-pan-zoom.enabled to false in about:config.

	Simon Lydell Reporter
	Comment 2 • 9 years ago

I don’t see the bug with APZ disabled. Thanks for the workaround.

	Mike Conley (:mconley) (:⚙️)
	Comment 3 • 9 years ago

Hey kats, this sounds like an APZ bug. I've marked this as blocking apz-desktop... is there anything else I need to do to make sure this bug is in the right place?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 9 years ago

Thanks for the test case! I can reproduce this as well. It seems very similar to bug 1217399, but is easier for me to reproduce. I'm changing it to block all-aboard-apz instead of apz-desktop because this is something we should fix before we ship APZ.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 9 years ago

I can start looking into this.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 9 years ago

I looked into this, and this is what I believe is happening when you hold down the 'k' key on the test case:

- the first smooth scroll request comes in to APZ and it starts the smooth scroll animation. the content-side scroll offset doesn't get updated yet
- additional smooth scroll requests come in to APZ with the same destination (which is based on the same content-side scroll offset). these can cancel the existing animation and start new ones. i have a patch locally that just updates the existing animation with the new destination, it doesn't matter.
- eventually the APZ animation comes to an end and it tries to send a repaint request to the main thread with the new final scroll offset
- however, every time the repaint request goes out, it gets rejected by the APZCCH code because the scroll generation number is stale, because the main thread is continually creating new smooth scroll requests and advancing the generation number
- this prevents the main thread from ever seeing the new scroll offset at the end of the animation, and so it keeps sending smooth scroll requests with the same destination.

Effectively the difference between the main-thread scrolling implementation and APZ for this use case is that the scroll generation number is perpetually rendered stale, and so APZ can't sync its new scroll offset to the main thread. The non-APZ implementation doesn't have this problem, so it works fine.

Trying to figure out what a good fix here is...

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 9 years ago

This fixes part of the problem at least. It stops the main thread from flooding APZ with the same request over and over unnecessarily. More importantly it stops increasing the scroll generation counter so APZ can actually send sync its scroll offset back to the main thread. The behaviour is better but still much worse compared to the up/down keys. I'll keep looking.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 9 years ago

And this should do the other bit. The remaining problem was that we kept restarting the compositor smooth scroll animation rather than just updating it to the new destination. Overall the scroll speed is still slower than the main-thread version but it's smooth at least.

	Simon Lydell Reporter
	Comment 9 • 9 years ago

I currently adjust the layout.css.scroll-behavior.spring-constant pref to change scroll speed. Are there any APZ prefs that make a difference as well?

Also, thanks for looking into this so quickly!

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 11 • 9 years ago

This is a little scary given how brittle the code is but I think this makes the code a bit clearer as to why things are happening in the order that they are.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 13 • 9 years ago

(In reply to Simon Lydell from comment #9)
> I currently adjust the layout.css.scroll-behavior.spring-constant pref to
> change scroll speed. Are there any APZ prefs that make a difference as well?
> 
> Also, thanks for looking into this so quickly!

The layout.css.scroll-behavior.damping-ratio pref works with layout.css.scroll-behavior.spring-constant to define the smooth-scrolling physics, and should affect both APZ and non-APZ versions. I don't think that any other APZ prefs will affect the speed of a single smooth-scroll.

	Botond Ballo [:botond]
	Comment 14 • 9 years ago

Comment on attachment 8696469 [details] [diff] [review]
Part 1 - Prevent layout from flooding APZ with smooth-scroll requests

Review of attachment 8696469 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2121,5 @@
>          }
>  
>          if (nsLayoutUtils::AsyncPanZoomEnabled(mOuter)) {
> +          if (mApzSmoothScrollDestination == mDestination &&
> +              mScrollGeneration == sScrollGenerationCounter) {

Couldn't sScrollGenerationCounter be incremented by a different scroll frame?

	Botond Ballo [:botond]
	Comment 15 • 9 years ago

Comment on attachment 8696471 [details] [diff] [review]
Part 2 - Rearrange some code/extract helper/update docs

Review of attachment 8696471 [details] [diff] [review]:
-----------------------------------------------------------------

The main change here seems to be that AcknowledgeScrollUpdate() is now called before CancelAnimation(). Is that significant?

	Botond Ballo [:botond]
	Comment 16 • 9 years ago

Comment on attachment 8696472 [details] [diff] [review]
Part 3 - Update smooth scroll animation destination rather than restarting it

Review of attachment 8696472 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but I'd like Kip to verify that calling SetDestination() on the axis models is sufficient and nothing else needs to be recomputed.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 9 years ago

(In reply to Botond Ballo [:botond] from comment #14)
> Couldn't sScrollGenerationCounter be incremented by a different scroll frame?

Hm, good point. Yeah it could but if that were to happen it would just fall back to the old behaviour so at least this patch doesn't make things worse. In order for the other scroll frame to severely impact the behaviour here, the two scrollframes would have to be getting scrolled while perfectly interleaved (e.g. the content is doing alternating calls to scrollBy on two different scrollframes). It doesn't seem like an entirely implausible scenario so we should probably handle it. I'll see if I can update the patch to do so.

(In reply to Botond Ballo [:botond] from comment #15)
> 
> The main change here seems to be that AcknowledgeScrollUpdate() is now
> called before CancelAnimation(). Is that significant?

Yes, sending the AcknowledgeScrollUpdate early means that any subsequent repaint requests (such as the one sent in CancelAnimation) will take effect on the main thread rather than getting rejected because of a stale generation counter. This new organization of code is basically another way to fix bug 1216355. The advantage is that the CancelAnimation/StartSmoothScroll (which are logically one operation) are grouped together. It's also easier to enforce the brittle requirement of not sending repaint requests between the CopySmoothScrollInfo and the scroll update acknowledgement (again, because they are grouped logically in the code).

(In reply to Botond Ballo [:botond] from comment #16)
> This looks fine, but I'd like Kip to verify that calling SetDestination() on
> the axis models is sufficient and nothing else needs to be recomputed.

Sure. For the record that's what the main thread code does as well - https://dxr.mozilla.org/mozilla-central/rev/59bc3c7a83de7ffb611203912a7da6ad84535a5a/layout/generic/nsGfxScrollFrame.cpp#2161

	:kip (Kearwood Gilbert)
	Comment 18 • 9 years ago

Comment on attachment 8696472 [details] [diff] [review]
Part 3 - Update smooth scroll animation destination rather than restarting it

Review of attachment 8696472 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

	Botond Ballo [:botond]
	Comment 19 • 9 years ago

Comment on attachment 8696469 [details] [diff] [review]
Part 1 - Prevent layout from flooding APZ with smooth-scroll requests

Review of attachment 8696469 [details] [diff] [review]:
-----------------------------------------------------------------

Feel free to leave the handling of multiple scroll frames being scrolled in an interleaved fashion, to a follow-up (maybe add a comment to mention it in that case).

	Pulsebot
	Comment 20 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ddca5b65d9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/32df7136da75
https://hg.mozilla.org/integration/mozilla-inbound/rev/9474cf8b8d18

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 21 • 9 years ago

Backed out for test failures in https://hg.mozilla.org/integration/mozilla-inbound/rev/407e7946bee4

There were at least some M-e10s-2 Linux x64 debug and B2G Emulator ICS opt M-8 tests failing, possibly more.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 22 • 9 years ago

I think I figured out the problem, the acknowledgement wasn't getting sent in the isFirstPaint case. Try push to see if there's other problems:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e2d0353300

	Pulsebot
	Comment 23 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a6158722a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8a815d5a1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d55129069a1f

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 24 • 9 years ago

Got r+ from botond for the incremental change at https://hg.mozilla.org/try/rev/75c97cf1dfd0, re-landed the patches with that.

	Phil Ringnalda (:philor)
	Comment 25 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f9a6158722a3
https://hg.mozilla.org/mozilla-central/rev/7a8a815d5a1c
https://hg.mozilla.org/mozilla-central/rev/d55129069a1f