1500565 - apz_test_native_event_utils.js's synthesizeNativeTouchSequences intermittently causes Assertion failure: !mTargetDuration.IsZero(), at /gfx/layers/apz/src/AndroidFlingPhysics.cpp:158

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Jan Henning [:JanH]]

Testing locally with a debug build on a real device, using [1] to simulate a touch drag scroll intermittently hits the assertion at [2].

For testing, you can import the patches for bug 1498812 from https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fd1d9f67c0807d29a4b686fc28ea13a406abd37 and test with https://hg.mozilla.org/try/annotate/dcc1f6bfc12f/mobile/android/tests/browser/chrome/test_session_scroll_visual_viewport.html.


[1] https://hg.mozilla.org/mozilla-central/annotate/c291143e24019097d087f9307e59b49facaf90cb/gfx/layers/apz/test/mochitest/apz_test_native_event_utils.js#l215
[2] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/gfx/layers/apz/src/AndroidFlingPhysics.cpp#158

Comment 1

[Botond Ballo [:botond]]

Curious. Looking at how mTargetDuration is computed, it seems the only way it would be zero is if the incoming mVelocity is zero, but we assert that three lines earlier.

Comment 2

[Botond Ballo [:botond]]

Hmm... in the Try push from comment 0, I retriggered the Android debug test chunk that runs test_session_scroll_visual_viewport.html (chunk c8) 10 times, but it didn't run into the assertion failure. I wonder if it's something that only happens locally, or perhaps it's just a really low-frequency intermittent?

Comment 3

[Jan Henning [:JanH]]

For me it wasn't *that* low-frequency when running locally, but it's possible that the timings on the emulator are different. Maybe this happens if the simulated events are sent too rapidly?

Comment 4

[Botond Ballo [:botond]]

Ok, I pulled the patches from Try and ran them locally, and triggered the bug after several retries.

When mTargetDuration is computed as 0, the incoming value of mVelocity was "inf". This suggests that we synthesized two touch events with different positions but the same timestamp.

Looking at the implementation of synthesizeNativeTouchSequences(), the events are not assigned a timestamp (and indeed, the nsIDOMWindowUtils.sendNativeTouchPoint() API that they use doesn't have such an option). Rather, the events pick up the current time as their timestamp in widget code.

This means that if the synthesization code runs sufficiently fast, we can end up with two subsequent events with the same timestamp. (This also explains why we don't see the bug in automation: the emulator is probably too slow for that to happen.)

Some possible fix options:

  1) Have synthesizeNativeTouchSequences() explicitly wait 1 ms or so
     between each touch point, so ensure they have distinct timestamps.

     This would be straightforward, but could slow down tests too much.

  2) Allow the synthesizing code to choose timestamps for the events
     (that don't have to correspond to wallclock-time).

     This would be the most general solution, but it would require
     plumbing the timestamp through a lot of places (nsIDOMWindowUtils,
     nsIWidget, various widget backends).

  3) Check for a velocity of "inf" in APZ code and don't start a fling
     in that case. Presumably, the tests that trigger this don't
     actually need a fling to take place, and if they do, they can
     do something else like option (1) above.

I'm leaning towards option (3), but I'm open to arguments otherwise.

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1500565 - Avoid start a fling animation with infinite velocity. r?kats

Comment 6

[Botond Ballo [:botond]]

The posted fix implements option (3).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58b7faab3b8d
Avoid start a fling animation with infinite velocity. r=kats

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/58b7faab3b8d

Comment 9

[Jan Henning [:JanH]]

Thank you.

Comment 10

[Botond Ballo [:botond]]

No worries, thanks for working on bug 1498812!