1157409 - APZ on Windows e10s loses some mouse wheel events with high-precision mouse

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Something crazy is happening with mousewheel scrolling today, some events are getting lost somehow. Intermittently my mouswheel scrolling doesn't do anything, at least not at first. Changing focus to other windows on my system and then back to firefox seems to make it happen more frequently.

Comment 1

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

Can you provide a specific page/scrollframe that didn't work?

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Also, what make/model mouse do you have?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It's a Logitech B100. And I see this behavior on bugzilla, gmail, everywhere.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

This is the hi-res mouse David was talking about, I believe.

Comment 5

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

This will need to be fixed before we turn it on by default.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

I got a high resolution mouse and can reproduce this really easily. It's pretty annoying.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Well, I definitely found one problem, a race condition. But I'm not sure if high resolution mice are related. Say the compositor thread (c) and controller thread (I) are separate threads, and we try to composite a frame as we get a new wheel event:

c: Begin a new frame.
c: Cache TimeStamp::Now() as APZ's current frame time.
I: Take the APZC lock to start an animation.
c: Take the APZC lock to update the current animation - BLOCKED.
I: Initialize a new wheel animation with TimeStamp::Now().
I: Release the APZC lock.
c: UNBLOCKED - the cached frame time is now < the animation start time.

This confuses the key spline function and we think the delta is 0, and end the animation prematurely.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

The problem is the IsZero(adjustedOffset) check in WheelScrollAnimation::DoSample(). Given that, this seemed like the easiest fix. I changed the animation to use GetFrameTime instead of event.mTimeStamp for good measure.

Comment 10

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

Comment on attachment 8597746 [details] [diff] [review]
fix

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

I'm fine with the WheelScrollAnimation changes, but I'd rather leave the Update call as using the timestamp from the event, unless we actually need it to fix the issue.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

The previous patch turned out to not work all the time. Even if we clamp the event time to <= |initial frame time|, the frame time could be so soon after the event that we still request a scroll delta of 0.

This version changes the condition that ends the animation, to exclude frames that result in an unadjusted displacement of 0.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb70f0ab7163

Comment 13

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

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