927033 - Touchmove events in a pinch sometimes have incorrect number of touch points

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

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

+++ This bug was initially created as a clone of Bug #927027 +++

The symptoms I was investigating are something like this:
1) Do a pinch-zoom
2) Put two fingers on the screen
3) Lift the two fingers from the screen

The expected results from steps 2-3 is that the page does not move or change in any way (it's a no-op pinch). The actual results are that the zoom of the page jumps drastically, usually right when you put your fingers on the screen.

I added some logging and found a couple of possible causes for this. The first one is filed as bug 927027, and the second is this.

The output I get with my logging is this:
---------------------
APZC got 2 touches with f=(1411 602) s=(1790 867) span=462.456482
APZC: 19CC0390 got a scale-begin in state 3
APZC got 3 touches with f=(1411 602) s=(1740 359) span=409.011017
APZC: 19CC0390 got a scale in state 8
APZC: span ratio is 409.011017 / 462.456482 == 0.884431
APZC got 2 touches with f=(1790 867) s=(1740 613) span=258.874481
APZC: 19CC0390 got a scale in state 8
APZC: span ratio is 258.874481 / 409.011017 == 0.632928
MetroWidget::ApzContentIgnoringTouch
APZC got 2 touches with f=(1790 861) s=(1740 359) span=504.483887
APZC: 19CC0390 got a scale in state 8
APZC: span ratio is 504.483887 / 258.874481 == 1.948759
APZC: Performed scale; new zoom is 2.719198
mozilla::widget::winrt::MetroInput::DispatchTouchCancel
APZC got 2 touches with f=(1790 867) s=(1752 605) span=264.741394
APZC: 19CC0390 got a scale in state 8[snip]
---------------

The interesting bit here is the third line of output, which says "got 3 touches" rather than "got 2 touches". The WidgetTouchEvent being delivered to the APZC has three touch points rather than two, even though i only put two fingers down. This might be bad hardware, but it might also be a bug in MetroInput.cpp which is adding extra touch points without removing old ones. It does some stuff with the pointer index and maintaining its own mTouches list that may or may not be ok. I will try to determine if this is a hardware problem or not.

Comment 1

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

The MetroInput.cpp mTouches variable has 2 touch points but the APZC gets 3. So it's definitely not a hardware problem but rather a bug in the code somewhere.

Comment 2

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

Additional logging shows that GestureEventListener::HandleEvent is getting called with a touch event that has two touch points, but the GestureEventListener::mTouches ends up having three touch points. It may be that MetroInput.cpp is sending touch events with an unexpected touch identifier, but regardless, this is a bug in GestureEventListener because it should be able to deal with bad input in a sane way.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

When I follow these steps:
1) Place two fingers on the screen
2) Remove both fingers

...two NS_TOUCH_END events are delivered from widget to APZCTreeManager, but only the first one is received by AsyncPanZoomController.  When it receives the first NS_TOUCH_END event, APZCTreeManager sets mApzcForInputBlock = nullptr, which causes it to ignore the second NS_TOUCH_END event.

Normally this isn't a problem because the dangling touch is removed from GestureEventListener::mTouches the next time it receives a touchmove.  But if you manage to avoid sending any touchmoves, that extra touch can remain in the list indefinitely.

We can fix this by adding a count of active pointers to APZCTreeManager, as this patch starts to do.  But it seems like maybe this count should be somewhere more global, or should be included in the touch event data.  (This is different from touchEvent.touches.Length() because "touches" has a different meaning for events like touchend and touchcancel.)

This patch also cleans up some warnings in GestureEventListener:

* We don't need to warn if we find an existing pointerId during a touchstart event.  This is expected, because the event is supposed to include all active pointers, not just the changed one.

* Made the warning for incorrect pointerId during touchend more correct, in the case where the event contains multiple touches.  This should never happen, but if we're going to loop through the touch list anyway we might as well do it correctly.  Maybe it would be better to just ASSERT(touches.length == 1).

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

forgot to qref my last round of edits

Comment 5

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

Comment on attachment 822622 [details] [diff] [review]
WIP 2

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

I don't really like this approach but I'm having a hard time thinking of a better one that doesn't involve much more significant changes. :(

I think the better solution is to implements pointer events, and have those dispatched by the widget code; the APZC would then be able to tell whether the last pointer went up or not without having to keep track of the count internally. I don't know what the timeline is for implementing pointer events though. Do you know?

Comment 6

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

Comment on attachment 822622 [details] [diff] [review]
WIP 2

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

Matt pointed out that pointerevents wouldn't help here either because they also don't provide information as to how whether or not all pointers are up or down.

Comment 7

[Jim Mathies [:jimm]]

blocking bug 915289 which is a bad zoom bug.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Comment 9

[Jim Mathies [:jimm]]

This patch has a bug in it with the current state of tip. When chrome consumes touch, MetroInput will call ContentReceivedTouch(true), but doesn't send touch point touchcancels to the apz. I can post a fix here in a bit.

Comment 10

[Jim Mathies [:jimm]]

Attachment: fix

This should land on fxteam.

Comment 11

[Matt Brubeck (:mbrubeck)]

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

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

::: widget/windows/winrt/MetroInput.cpp
@@ +1218,5 @@
>    if (!touchEvent.touches.Length()) {
>      return;
>    }
> +  if (mContentConsumingTouch) {
> +    DUMP_TOUCH_IDS("APZC(4)", &touchEvent);

Are these dump statements meant to be checked in?

Comment 12

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #11)
> Comment on attachment 826054 [details] [diff] [review]
> fix
> 
> Review of attachment 826054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/winrt/MetroInput.cpp
> @@ +1218,5 @@
> >    if (!touchEvent.touches.Length()) {
> >      return;
> >    }
> > +  if (mContentConsumingTouch) {
> > +    DUMP_TOUCH_IDS("APZC(4)", &touchEvent);
> 
> Are these dump statements meant to be checked in?

yes, that code is currently on fx team.

Comment 13

[Jim Mathies [:jimm]]

I'm seeing a situation on facebook where the count gets out of sync (mTouchCount is > 0 after the last touch leaves the screen).

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #13)
> I'm seeing a situation on facebook where the count gets out of sync
> (mTouchCount is > 0 after the last touch leaves the screen).

I can reproduce this on any page by flicking the page up and down repeatedly. Trying to track down a cause.

Comment 15

[Jim Mathies [:jimm]]

Here's the sequence that does it - 

DumpTouchIds: NS_TOUCH_START block
   id=1757 target=APZC(1)
ReceiveInputEvent(1): add 1 mTouchCount = 1
DumpTouchIds: NS_TOUCH_START block
   id=1757 target=DOM(2)
MetroWidget::ApzContentIgnoringTouch
DumpTouchIds: NS_TOUCH_CANCEL block
   id=1757 target=DOM(4)
DumpTouchIds: NS_TOUCH_END block
   id=1757 target=APZC(3)
DumpTouchIds: NS_TOUCH_START block
   id=1758 target=APZC(1)
ReceiveInputEvent(1): add 1 mTouchCount = 2

We never get a corresponding decrement of mTouchCount for the touchend. I'm guessing this is due to a failure in looking up the target apzc in which case we return early from ReceiveInputEvent without calling ProcessTouchEvent for the event.

I've seen this before, I think GetTouchInputBlockAPZC can occasionally fail for unknown reasons.

Comment 16

[Jim Mathies [:jimm]]

Attachment: debug output patch

Comment 17

[Jim Mathies [:jimm]]

Can we go ahead and get this landed? I'm in need of some form of touch block tracking in the tree manager or controller for some other work.

Comment 18

[Matt Brubeck (:mbrubeck)]

Attachment: followup part 1: move some duplicate code into ProcessTouchEvent

This is a pure refactoring to merge some exact duplicate code.

Comment 19

[Matt Brubeck (:mbrubeck)]

Attachment: followup part 2: Always decrement mTouchCount

This should fix the bug noted in comment 13.  With this patch, we decrement mTouchCount whether or not we find an APZC.

Note: All the re-indented lines in this patch are whitespace-only changes except for the line where "ret" was originally declared.

Comment 20

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/e702cd7761e5
https://hg.mozilla.org/integration/fx-team/rev/a5cf7ec69d76
https://hg.mozilla.org/integration/fx-team/rev/ba0ad4302495
https://hg.mozilla.org/integration/fx-team/rev/1afc9420addc

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/e702cd7761e5
https://hg.mozilla.org/mozilla-central/rev/a5cf7ec69d76
https://hg.mozilla.org/mozilla-central/rev/ba0ad4302495
https://hg.mozilla.org/mozilla-central/rev/1afc9420addc

Comment 23

[Manuela Muntean [Away]]

Verified as fixed with Firefox 28 beta 6 on a Dell XPS 12 ultrabook with Windows 8 64-bit.