1284570 - Android APZ fling can end up with a NaN velocity

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P1)

Description

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

Attachment: Log snippet

In bug 1279299 a user reported some checkerboarding badness. While investigating we got a lot that indicated NaNs were being produced in the code and probably screwing things up badly. I'm attaching a relevant snippet of the log, where the first nan value shows up. It shows up in the x-component of the APZC's velocity, right after a fling animation starts.

Based on code inspection, I suspect what's happening is that the android code at [1] sets mSplineDuration to 0, and since the x-velocity is 0 to start off, the code at [2] leaves it at 0. Then on the update at [3] it divides by that 0 and produces a NaN. That's consistent with the log, where the fling's initial x component is 0.

[1] http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/mobile/android/base/java/org/mozilla/gecko/gfx/StackScroller.java#492
[2] http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/mobile/android/base/java/org/mozilla/gecko/gfx/StackScroller.java#504
[3] http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/mobile/android/base/java/org/mozilla/gecko/gfx/StackScroller.java#659

Comment 1

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

Randall said he would take this.

Comment 2

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1284570-Android-APZ-fling-can-end-up-with-a-NaN-velocity-r-snorp-16070514-c2bf60e.patch

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8768190 [details] [diff] [review]
0001-Bug-1284570-Android-APZ-fling-can-end-up-with-a-NaN-velocity-r-snorp-16070514-c2bf60e.patch

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

Maybe try to get this upstream too?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25979328f745
Android APZ fling can end up with a NaN velocity r=snorp

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25979328f745

Comment 6

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

Marking 49 as unaffected because bug 1280666 isn't on that branch. If we uplift that then this should get uplifted as well.

Comment 7

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

Bug 1280666 was approved for 49, so we will need to uplift this as well. Randall, can you request uplift?

Comment 8

[Randall Barker [:rbarker]]

Comment on attachment 8768190 [details] [diff] [review]
0001-Bug-1284570-Android-APZ-fling-can-end-up-with-a-NaN-velocity-r-snorp-16070514-c2bf60e.patch

Approval Request Comment
[Feature/regressing bug #]: NaNs are generated by fling animation causing checker boarding. This is a regression caused by Bug 1280666 which was just uplifted.
[User impact if declined]: APZ will checkerboard more often and screen sometimes goes blank.
[Describe test coverage new/current, TreeHerder]: mostly manual.
[Risks and why]: Minimal. Uplifting Bug 1280666 was the bigger risk, this fixes an issue with that patch.
[String/UUID change made/needed]: none.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8768190 [details] [diff] [review]
0001-Bug-1284570-Android-APZ-fling-can-end-up-with-a-NaN-velocity-r-snorp-16070514-c2bf60e.patch

Patch has been in Nightly50 for almost a month, seems small in size, it's still start of the Beta cycle, taking it. Beta49+

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/900bcde62614