964675 - Making pinch and preventing default behavior of first touchstart event sometimes produce wrong sequence of touch events on Firefox for metro.

Status: RESOLVED WONTFIX

(Core Graveyard :: Widget: WinRT, defect)

Description

[Nick Lebedev [:nl]]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.76 Safari/537.36

Steps to reproduce:

In this bug I would like to note a potential problem that happen currently in the metroinput.cpp code.

In the metroinput.cpp we’re having the following event handling model: 
  * we catch native pointer events from OS and create widget events from them [1]
  * after that we schedule dispatching of such events [2]

In both methods we rely on the mCancelable variable that may depend on the dispatch delay. E.g. we have pinch and therefore two calls of OnPointerPressed, in the first call we have mCancelable set to true but in the second call we have mCancelable dropped to the false [3].

If we have [2] called after two calls of [1] we get mCancelable equal to false but if we have [2] called in between of two calls of [1] we get mCancelable equal to true. This race leads to different handling of pinch (or maybe other multi touch inputs). Currently it doesn't seem to break something but may in future.

[1] MetroInput::OnPointerPressed, http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#532

[2] MetroInput::DeliverNextQueuedTouchEvent, http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#1316

[3] http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#577

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

jmathies, I'm checking through the UNCONFIRMED bugs reported for Firefox 28, and I am not set up to confirm or otherwise triage this bug. Does the metro team have any particular tags you're using to track bugs like this? I will also ask around in QA to see who should be cc-ed on the bug. Thanks!

Comment 2

[Jim Mathies [:jimm]]

(In reply to Liz Henry :lizzard from comment #1)
> jmathies, I'm checking through the UNCONFIRMED bugs reported for Firefox 28,
> and I am not set up to confirm or otherwise triage this bug. Does the metro
> team have any particular tags you're using to track bugs like this? I will
> also ask around in QA to see who should be cc-ed on the bug. Thanks!

If it's something new we would add it to a triage list which we go over every week. If we want to get it fixed in a particular train we would tag it accordingly. We add [metro] to platform bugs in places like layout and gfx to keep track of stuff outside our product. (There's no need for the metro flag in widget/winrt since everything in here is metro related.)

triage tag: [triage]
train target: r=ff30
general tracking in platform: [metro] 

I'll mark this for triage again now that we've decided not to fix bug 956644.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks very much, Jim!

Comment 4

[Nick Lebedev [:nl]]

Attachment: Moved state variables initialization to the delayed method DeliverNextQueuedTouchEvent.

Jim, I also would like to add a test for my changes. That would be an html test that imitates a pinch and checks that corrected sequence of touch events is received.
Could you let me know what would be the best place for such test? - dom/events/test or some metro specific folder?

Comment 5

[Nick Lebedev [:nl]]

Found the impact of this issue:
Sometimes when i perform pinch and call e.preventDefault on touchstart event firefox produces start/move/cancel events but should start/move/end.
Due to the found defect i'm renaming the bug.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8380497 [details] [diff] [review]
Moved state variables initialization to the delayed method DeliverNextQueuedTouchEvent.

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

::: widget/windows/winrt/MetroInput.cpp
@@ +1340,5 @@
> +    mApzConsumingTouch = false;
> +    mCancelable = true;
> +    mCanceledIds.Clear();
> +  } else {
> +    mCancelable = false;

Should we reset this on the first touch event after the first touch start? This doesn't seem right.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Nick Lebedev [:nl] from comment #4)
> Created attachment 8380497 [details] [diff] [review]
> Moved state variables initialization to the delayed method
> DeliverNextQueuedTouchEvent.
> 
> Jim, I also would like to add a test for my changes. That would be an html
> test that imitates a pinch and checks that corrected sequence of touch
> events is received.
> Could you let me know what would be the best place for such test? -
> dom/events/test or some metro specific folder?

I think the only place you can do this today would be in metro mochitest - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/

Comment 8

[Nick Lebedev [:nl]]

Attachment: Moved state variables initialization to the delayed method DeliverNextQueuedTouchEvent. v2.

Carrying over the r+ from jimm.

To Jim: 
1. How do you think - should we move mRecognizerWantsEvents variable together with other state variables?

2. I've update patch according to your comment but got stuck with tests. To properly test this patch it would be great to simulate pinch and to check sequence of generated touch events. Seems like metro tests don't allow to check what touch events are dispatched (correct me if i'm wrong). Maybe i should proceed with dom/events tests but i'm not sure whether we can simulate a pinch in such tests. Or I maybe there is some other way to create test coverage for such patch. Could you let me know what next steps you would prefer?

Comment 9

[Jim Mathies [:jimm]]

(In reply to Nick Lebedev [:nl] from comment #8)
> Created attachment 8385486 [details] [diff] [review]
> Moved state variables initialization to the delayed method
> DeliverNextQueuedTouchEvent. v2.
> 
> Carrying over the r+ from jimm.
> 
> To Jim: 
> 1. How do you think - should we move mRecognizerWantsEvents variable
> together with other state variables?
> 
> 2. I've update patch according to your comment but got stuck with tests. To
> properly test this patch it would be great to simulate pinch and to check
> sequence of generated touch events. Seems like metro tests don't allow to
> check what touch events are dispatched (correct me if i'm wrong). Maybe i
> should proceed with dom/events tests but i'm not sure whether we can
> simulate a pinch in such tests. Or I maybe there is some other way to create
> test coverage for such patch. Could you let me know what next steps you
> would prefer?

Is it ok if the html page that receives the events consumes them? If so the mohcitest can inject the input, and the html page loaded in a tab can send an event to the mochitest telling it what it received. The mochitest can wait for the expected result from the page. I can point you to a test that does this, but I'm curious if you're trying to test event flow or something the apzc supposed to do..

Comment 10

[Jim Mathies [:jimm]]

(In reply to Nick Lebedev [:nl] from comment #8)
> Created attachment 8385486 [details] [diff] [review]
> Moved state variables initialization to the delayed method
> DeliverNextQueuedTouchEvent. v2.
> 
> Carrying over the r+ from jimm.
> 
> To Jim: 
> 1. How do you think - should we move mRecognizerWantsEvents variable
> together with other state variables?

if it's working correctly lets leave it alone. This code is going through a simplification process anyway with the input thread separation work.

Comment 11

[Nick Lebedev [:nl]]

Resolving since the code affected by my patch has already passed through huge simplification process.