Closed Bug 1284570 Opened 4 years ago Closed 4 years ago

Android APZ fling can end up with a NaN velocity

Categories

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

50 Branch
All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox48 --- unaffected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: kats, Assigned: rbarker)

References

Details

Attachments

(2 files)

Attached file 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
Randall said he would take this.
Assignee: nobody → rbarker
Priority: -- → P1
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?
Attachment #8768190 - Flags: review?(snorp) → review+
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25979328f745
Android APZ fling can end up with a NaN velocity r=snorp
https://hg.mozilla.org/mozilla-central/rev/25979328f745
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Marking 49 as unaffected because bug 1280666 isn't on that branch. If we uplift that then this should get uplifted as well.
Bug 1280666 was approved for 49, so we will need to uplift this as well. Randall, can you request uplift?
Flags: needinfo?(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.
Flags: needinfo?(rbarker)
Attachment #8768190 - Flags: approval-mozilla-aurora?
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+
Attachment #8768190 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.