1326686 - [e10s] Page in background window doesn't scroll smoothly when I click on its scrollbar

Status: VERIFIED FIXED

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

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url [1] or url [2] in a new selected tab in Firefox window
2. Dock the window to the Right side of the screen (e.g. Win+Right)
3. Open new Firefox window, dock it to the Left side of the screen (Win+Left), click in that window
4. Click on empty area in scrollbar in the tab from Step 1

AR:  Page content just teleports to new position, scrollbar teleports to new position
ER:  Page content should smoothly scroll to new position. Scrollbar should move smoothly too

> [1] data:text/html,<style>body{height:10000px;background: linear-gradient(gray, lightgray);background-size: 700px 700px;}
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1272816

Comment 1

[Ciprian Muresan [:cmuresan], Ecosystem QA]

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID 	20170116030326

The issue is reproducible on the latest Firefox release (50.1.0) and on the latest Nightly (53.0a1).

Comment 2

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

That's a pretty interesting bug. Happens with and without APZ. Also the two windows are not strictly required - it seems to happen any time you click on the scrollbar track while the window doesn't have focus. Looking at the code in nsSliderFrame.cpp [1] it doesn't look obviously focus-dependent. This'll need further investigation.

[1] http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/xul/nsSliderFrame.cpp#588

Comment 3

[Botond Ballo [:botond]]

I can't repro this on Linux - I'm guessing a Windows-specific issue?

Comment 4

[Botond Ballo [:botond]]

Never mind, I misread the STR. The difference between the two behaviours is "scroll to click" (actual) vs. "scroll in direction of click" (expected). On Linux we always do "scroll to click", even if the windows has focus, so there is no difference between the focused and non-focused cases.

Comment 5

[Botond Ballo [:botond]]

Nope, wrong again. "Scroll to click" was a red herring. With scroll to click disabled if applicable, the expected behaviour is "scroll in the direction of click with a smooth animation", and the actual behaviour is "scroll in the direction of click immediately", and I can reproduce this on both Windows and Linux.

Comment 6

[Botond Ballo [:botond]]

The issue is related to the fact that when the main thread sets up a smooth scroll animation (AsyncScroll), the start time is the refresh driver's most recent refresh time [1].

If the window is not focused, the most recent refresh time can be quite a while ago, so by the time we sample by smooth scroll animation, its computed target end time has already arrived, and we just jump to the final position.

[1] http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/generic/nsGfxScrollFrame.cpp#2285

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)
> The issue is related to the fact that when the main thread sets up a smooth
> scroll animation (AsyncScroll), the start time is the refresh driver's most
> recent refresh time [1].
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 790b2cb423ea1ecb5746722d51633caec9bab95a/layout/generic/nsGfxScrollFrame.
> cpp#2285

This behaviour was introduced in bug 1022818, with bug 1022818 comment 60 hinting at a justification:

> - Fixed bug in nsGfxScrollFrame which caused erratic behavior when
> mochitests took control of the refresh driver (Was using TimeStamp::Now()
> rather than getting the time from the refresh driver.

Kip, do you have a suggestion for what we could do here that would both keep your tests green and avoid this functional regression?

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> > - Fixed bug in nsGfxScrollFrame which caused erratic behavior when
> > mochitests took control of the refresh driver (Was using TimeStamp::Now()
> > rather than getting the time from the refresh driver.
> 
> Kip, do you have a suggestion for what we could do here that would both keep
> your tests green and avoid this functional regression?

Since the concern is specific to the case where a test is controlling the refresh driver, we can restrict the behaviour change to that case.

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1326686 - Only use the most recent refresh time as the start time of an AsyncScroll when the refresh driver is under test control.

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

Comment 10

[:kip (Kearwood Gilbert)]

Comment on attachment 8829009 [details]
Bug 1326686 - Only use the most recent refresh time as the start time of an AsyncScroll when the refresh driver is under test control.

https://reviewboard.mozilla.org/r/106222/#review107718

This looks good to me, thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b9e53e7eee7
Only use the most recent refresh time as the start time of an AsyncScroll when the refresh driver is under test control. r=kip

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/f0ead5f455ed - not sure if it also caused failures on other platforms and we just misstarred them, but linux64 debug e10s, https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=9c8fc27b018c23da75a9529615b2d94282308da4&group_state=expanded&filter-searchStr=fc9810f76c8215b1c0e8153f7f2d6b45b472a767&tochange=a70d8500b13aa2bb046a65e417a836f22d15d4cc it caused quite frequent failures in layout/base/tests/test_scroll_snapping_scrollbars.html

Comment 13

[Botond Ballo [:botond]]

Thanks. I did do a Try run [1] where the test in question passed, but after retriggering it multiple times I do see it intermittently failing.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=73e7b2d116a6

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

as a note this caused a perf regression:
== Change summary for alert #4884 (as of January 24 2017 01:48 UTC) ==

Regressions:

  5%  tp5o_scroll summary windows8-64 opt     3.42 -> 3.6
  3%  tp5o_scroll summary windows7-32 pgo     2.79 -> 2.88

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4884

Comment 15

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 16

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Botond Ballo [:botond] from comment #6)
> The issue is related to the fact that when the main thread sets up a smooth
> scroll animation (AsyncScroll), the start time is the refresh driver's most
> recent refresh time [1].
> 
> If the window is not focused, the most recent refresh time can be quite a
> while ago, so by the time we sample by smooth scroll animation, its computed
> target end time has already arrived, and we just jump to the final position.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 790b2cb423ea1ecb5746722d51633caec9bab95a/layout/generic/nsGfxScrollFrame.
> cpp#2285

I ran into this problem again with bug 1377814. Scheduling empty transactions more frequently made this issue worse. I've discussed this with Botond and we'd both like to get this fixed.

I've done a new mochitest try run with Botond's patch and retriggered the job multiple times that was intermittently failing originally. It looks green to me now. I wasn't able to reproduce it locally either.

I'm unsure if the issue has been fixed or if it is still around.

Comment 17

[Botond Ballo [:botond]]

(In reply to Ryan Hunt [:rhunt] from comment #16)
> I've done a new mochitest try run with Botond's patch and retriggered the
> job multiple times that was intermittently failing originally. It looks
> green to me now. I wasn't able to reproduce it locally either.

Interesting. I guess something else must have changed in the meantime, that made that failure disappear.

I would suggest we go ahead and re-land the patch.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2851f5df207
Only use the most recent refresh time as the start time of an AsyncScroll when the refresh driver is under test control. r=kip

Comment 19

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

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

Comment 20

[Ciprian Muresan [:cmuresan], Ecosystem QA]

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID 	20170711030203

The issue is verified fixed on the latest Nightly 56.0a1 (2017-07-11) on the following OSes:
- Windows 10 x64
- Windows 8.1 x64