918273 - ContentReceivedTouch doesn't actually cancel event processing in the apzc

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Jim Mathies [:jimm]]

This looks like a bug, so filing to investigate. In ReceiveInputEvent we identify the apz that will be the target of touch input and store it in mApzcForInputBlock  -

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#426

If a caller decides to cancel the touch sequence, the tree manager delivers the ContentReceivedTouch call to an apz based a guid. In the metro case, this would be the root apz.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#426

I think ContentReceivedTouch should be calling ContentReceivedTouch on the apz stored in mApzcForInputBlock if it exists.

Kats any thoughts?

Comment 1

[Jim Mathies [:jimm]]

Also it's not clear to me what AsyncPanZoomController::ContentReceivedTouch is trying to do.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1317

Assume we call ContentReceivedTouch w/aPreventDefault = true when touchstart or the first touchmove have preventDefault called on them. If we continue to send input to the tree manager, it looks like that input will still be delivered and processed by an apz. I would expect that the only processing needed past that point would be event coordinate transformation in the tree manager.

Comment 2

[Jim Mathies [:jimm]]

Just confirmed that if continue to send events after calling ContentReceivedTouch w/aPreventDefault = true on a touch start or first touch move the apz continues to react to input. So this needs to be updated. Alternatively I can add a new tree manager method for transforming event data that doesn't forward input input to the apz. Then i can handle this in widget.

Comment 3

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

(In reply to Jim Mathies [:jimm] from comment #0)
> If a caller decides to cancel the touch sequence, the tree manager delivers
> the ContentReceivedTouch call to an apz based a guid. In the metro case,
> this would be the root apz.
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> APZCTreeManager.cpp#426
> 
> I think ContentReceivedTouch should be calling ContentReceivedTouch on the
> apz stored in mApzcForInputBlock if it exists.
> 
> Kats any thoughts?

Yes, this appears to be a bug. Sending it to mApzcForInputBlock will work some of the time, but it's possible for mApzcForInputBlock to be nulled out before the ContentReceivedTouch call comes in, so we'll need something a bit more robust to handle this scenario.

(In reply to Jim Mathies [:jimm] from comment #2)
> Just confirmed that if continue to send events after calling
> ContentReceivedTouch w/aPreventDefault = true on a touch start or first
> touch move the apz continues to react to input. So this needs to be updated.

Agreed, this needs to be updated. I was just looking at the code and it doesn't appear to handle this case at all, but I was assuming it did. To fix this I would like to add some code to AsyncPanZoomController to basically drop the rest of the events in the "touch event block" (as defined at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java#26) if aPreventDefault is true.

> Alternatively I can add a new tree manager method for transforming event
> data that doesn't forward input input to the apz. Then i can handle this in
> widget.

I would prefer to avoid doing this.

Comment 4

[Jim Mathies [:jimm]]

Went digging for uses of ContentReceivedTouch. The only real use I could find was in TabChild here - 

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#1583
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1847

Which looks like it gets called on every touch event except touch moves, which are handled in another method. RecvRealTouchEvent hands back gPreventMouseEvents which looks right to me in terms of how it gets set -

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7001

(Kinda gross that we use a global bool for this.)

Also it looks like the guid passed in from RenderFrameParent is associated with the foreground tab - 

http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#1030

Comment 5

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Yes, this appears to be a bug. Sending it to mApzcForInputBlock will work
> some of the time, but it's possible for mApzcForInputBlock to be nulled out
> before the ContentReceivedTouch call comes in, so we'll need something a bit
> more robust to handle this scenario.

Hmm, I don't see the issue here. The only place we null out mApzcForInputBlock is on a MULTITOUCH_CANCEL or MULTITOUCH_END, in which case the touch start and touch move have already been processed.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#265

If we are more explicit in the definition of ContentReceivedTouch (must be called on touch start or the first touch move) then we should be ok using mApzcForInputBlock.

Comment 6

[Jim Mathies [:jimm]]

So in working on this I've found that on metro mFrameMetrics.mMayHaveTouchListeners is always false for the layer we scroll (and for the discovered mApzcForInputBlock). So we never get to state WAITING_LISTENERS and can't cancel at all.

Comment 7

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

(In reply to Jim Mathies [:jimm] from comment #5)
> Hmm, I don't see the issue here. The only place we null out
> mApzcForInputBlock is on a MULTITOUCH_CANCEL or MULTITOUCH_END, in which
> case the touch start and touch move have already been processed.
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> APZCTreeManager.cpp#265
> 
> If we are more explicit in the definition of ContentReceivedTouch (must be
> called on touch start or the first touch move) then we should be ok using
> mApzcForInputBlock.

I guess on Metro this is ok because the input events are sent to APZC on the gecko thread, but this is not true across all platforms. On Android, the flow could look like this:

UI thread sends MULTITOUCH_START to APZC
UI thread hands untransformed MULTITOUCH_START to Gecko thread
UI thread sends MULTITOUCH_MOVE to APZC
UI thread hands untransformed MULTITOUCH_MOVE to Gecko thread
UI thread sends MULTITOUCH_END to APZC (mApzcForInputBlock is nulled out)
UI thread hands untransformed MULTITOUCH_END to Gecko thread
Gecko thread processes MULTITOUCH_START, sends ContentReceivedTouch(true) to APZC

In this flow the call to ContentReceivedTouch would have a null mApzcForInputBlock and would not get delivered properly. On Metro all of this happens on the Gecko thread so it's not a problem.

Comment 8

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

(In reply to Jim Mathies [:jimm] from comment #6)
> So in working on this I've found that on metro
> mFrameMetrics.mMayHaveTouchListeners is always false for the layer we scroll
> (and for the discovered mApzcForInputBlock). So we never get to state
> WAITING_LISTENERS and can't cancel at all.

That seems wrong.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > Hmm, I don't see the issue here. The only place we null out
> > mApzcForInputBlock is on a MULTITOUCH_CANCEL or MULTITOUCH_END, in which
> > case the touch start and touch move have already been processed.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> > APZCTreeManager.cpp#265
> > 
> > If we are more explicit in the definition of ContentReceivedTouch (must be
> > called on touch start or the first touch move) then we should be ok using
> > mApzcForInputBlock.
> 
> I guess on Metro this is ok because the input events are sent to APZC on the
> gecko thread, but this is not true across all platforms. On Android, the
> flow could look like this:
> 
> UI thread sends MULTITOUCH_START to APZC
> UI thread hands untransformed MULTITOUCH_START to Gecko thread
> UI thread sends MULTITOUCH_MOVE to APZC
> UI thread hands untransformed MULTITOUCH_MOVE to Gecko thread
> UI thread sends MULTITOUCH_END to APZC (mApzcForInputBlock is nulled out)
> UI thread hands untransformed MULTITOUCH_END to Gecko thread
> Gecko thread processes MULTITOUCH_START, sends ContentReceivedTouch(true) to
> APZC
> 
> In this flow the call to ContentReceivedTouch would have a null
> mApzcForInputBlock and would not get delivered properly. On Metro all of
> this happens on the Gecko thread so it's not a problem.

Hmm, I didn't realize there was that much delay. Seems like the simplest solution then would be to keep mApzcForInputBlock around longer and find a more appropriate time to clear it, if that's even necessary.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #9)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> > UI thread sends MULTITOUCH_START to APZC
> > UI thread hands untransformed MULTITOUCH_START to Gecko thread
> > UI thread sends MULTITOUCH_MOVE to APZC
> > UI thread hands untransformed MULTITOUCH_MOVE to Gecko thread
> > UI thread sends MULTITOUCH_END to APZC (mApzcForInputBlock is nulled out)
> > UI thread hands untransformed MULTITOUCH_END to Gecko thread
> > Gecko thread processes MULTITOUCH_START, sends ContentReceivedTouch(true) to
> > APZC
> > 
> > In this flow the call to ContentReceivedTouch would have a null
> > mApzcForInputBlock and would not get delivered properly. On Metro all of
> > this happens on the Gecko thread so it's not a problem.
> 
> Hmm, I didn't realize there was that much delay. Seems like the simplest
> solution then would be to keep mApzcForInputBlock around longer and find a
> more appropriate time to clear it, if that's even necessary.

We might have to tie mApzcForInputBlock to the input batch somehow I think, if there's a chance this could wrap. e.g. we might get 

UI thread sends MULTITOUCH_START(1) to APZC
UI thread hands untransformed MULTITOUCH_START to Gecko thread
UI thread sends MULTITOUCH_MOVE to APZC
UI thread hands untransformed MULTITOUCH_MOVE to Gecko thread
UI thread sends MULTITOUCH_END to APZC (mApzcForInputBlock is nulled out)
UI thread hands untransformed MULTITOUCH_END to Gecko thread
UI thread sends MULTITOUCH_START(2) to APZC
Gecko thread processes MULTITOUCH_START(1), sends ContentReceivedTouch(true) to APZC

Comment 11

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > So in working on this I've found that on metro
> > mFrameMetrics.mMayHaveTouchListeners is always false for the layer we scroll
> > (and for the discovered mApzcForInputBlock). So we never get to state
> > WAITING_LISTENERS and can't cancel at all.
> 
> That seems wrong.

Yeah I'm going to try debugging nsDisplayList tomorrow, see what's going on.

Comment 12

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

(In reply to Jim Mathies [:jimm] from comment #10)
> We might have to tie mApzcForInputBlock to the input batch somehow I think,
> if there's a chance this could wrap. e.g. we might get 

Yes, it could wrap. One option is to return the ScrollableLayerGuid of the APZC back out along with the untransformed event, and then the caller can ensure that they pass it back in when calling ContentReceivedTouch. That way we can guarantee it gets the right APZC.

Comment 13

[Jim Mathies [:jimm]]

Still trying to understand what's going on here. Doing soem logging in nsDisplayList and down in APZTreeManager, I see two distinct layer trees being build, and we seem to be attaching to the wrong one. 

For the main browser:

> PaintRoot
dl  container: A3D3DA8 touch: 0 scrollid: 1 uri: 
dl  container: A3D5F58 touch: 1 scrollid: 0 uri: chrome://browser/content/browser.xul
> UpdatePanZoomControllerTree
apz container: A6CD4B8 touch: 1 scrollid: 0 scrollable: 0 uri: chrome://browser/content/browser.xul
apz container: A9005B8 touch: 0 scrollid: 0 scrollable: 0 uri: 
apz container: ED208F0 touch: 0 scrollid: 0 scrollable: 0 uri: 
apz container: 9970058 touch: 0 scrollid: 1 scrollable: 1 uri: 
controller found for container: 9970058
< UpdatePanZoomControllerTree
< PaintRoot

This is odd since I can pan content, but afaict our controller isn't attached to the browser frame. Note I'm pausing PaintRoot here to give the compositor thread time to update within the same time segment.

Immediately after this in display list I see another PaintRoot call - 

> PaintRoot
dl  container: A3D4C18 touch: 1 scrollid: 1 uri: https://bug915213.bugzilla.mozilla.org/attachment.cgi?id=803131
< PaintRoot

which is my touch demo loaded in a browser tab. The compositor doesn't update here at all. Not sure whats going on, but will continue to dig.

Comment 14

[Jim Mathies [:jimm]]

Dumping the frame type helps a bit here - 

> PaintRoot
dl  container: A0FA3C0 touch: 0 scrollid: 1 type: CanvasFrame
dl  container: 4BE85C0 touch: 1 scrollid: 0 type: ViewportFrame
> UpdatePanZoomControllerTree
apz container: 9D0DA30 touch: 1 scrollid: 0 scrollable: 0 type: ViewportFrame
apz container: A0FED68 touch: 0 scrollid: 0 scrollable: 0 type: 
apz container: 95CC5C0 touch: 0 scrollid: 0 scrollable: 0 type: 
apz container: A0EF1D0 touch: 0 scrollid: 1 scrollable: 1 type: CanvasFrame
controller found for container: A0EF1D0
< UpdatePanZoomControllerTree
< PaintRootntRoot

I do not understand why the controller is attached to a CanvasFrame instead of ViewportFrame. I'm not even sure where the canvas frame comes from, although we do have a canvas in our nav bar.

Comment 15

[Jim Mathies [:jimm]]

Attachment: dump-58.html

I did a dump of the paint list and things look OK. Although we still have this issue where a page that listens for touch does not set mMayHaveTouchListeners in frame metrics. I'm going to split that off from this work.

Comment 16

[Jim Mathies [:jimm]]

Attachment: fix ContentReceivedTouch v.1

Comment 17

[Jim Mathies [:jimm]]

> Agreed, this needs to be updated. I was just looking at the code and it
> doesn't appear to handle this case at all, but I was assuming it did. To fix
> this I would like to add some code to AsyncPanZoomController to basically
> drop the rest of the events in the "touch event block" (as defined at
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/
> TouchEventHandler.java#26) if aPreventDefault is true.

Note this patch addresses this issue, it doesn't address the nulled out mApzcForInputBlock when events wrap. I'll file a follow up for that.

Comment 18

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

Comment on attachment 822527 [details] [diff] [review]
fix ContentReceivedTouch v.1

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

Sorry for the delayed review; I had to re-familiarize myself with some of this code. This looks fine, although some of the code you're touching is dead and should be removed (bug 906877).

Comment 19

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> Comment on attachment 822527 [details] [diff] [review]
> fix ContentReceivedTouch v.1
> 
> Review of attachment 822527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delayed review; I had to re-familiarize myself with some of
> this code. This looks fine, although some of the code you're touching is
> dead and should be removed (bug 906877).

I had a feeling this was the case since BrowserElementPanning is going away here pretty soon?

Comment 20

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

Some bits of BrowserElementPanning can be removed now (the parts that deal with stealing subframe scrolling from APZC) but a lot of it is still used on B2G. That will eventually go away as well but probably not in Gecko 28.

Comment 21

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

I just landed bug 906877 on b2g-inbound which I belated realized also removes the CancelDefaultPanZoom function from AsyncPanZoomController.cpp. Feel free to resurrect it as a private function in AsyncPanZoomController since you still need it for this patch.

Comment 22

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> I just landed bug 906877 on b2g-inbound which I belated realized also
> removes the CancelDefaultPanZoom function from AsyncPanZoomController.cpp.
> Feel free to resurrect it as a private function in AsyncPanZoomController
> since you still need it for this patch.

My patch is already on fx-team. :)

Comment 23

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/ef51f8a92b09

backed out, I'll clean this up whenever the trees open.

Comment 24

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

Whoops, sorry! I didn't see the fx-team landing. Thanks for backing yours out although I'd have been fine with you backing mine out :)

Comment 25

[Jim Mathies [:jimm]]

Attachment: fix ContentReceivedTouch v.2

This adds back the touch block cancelation, and also adds good touch block tracking. One of the things I've noticed with this tracking is that multi-touch can leave the apz in an unexpected state which might be the cause of bugs like bug 915289 and bug 927027.

Comment 26

[Jim Mathies [:jimm]]

Attachment: metroinput touch point debugging v.1

Some useful debugging routines for our touch event sending. Right now everything looks great.

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 825328 [details] [diff] [review]
fix ContentReceivedTouch v.2

Hmm, so looks like GestureEventListener can filter out multitouch before it reaches the touch handlers, so I'm going to have to account for that as well.

Comment 28

[Jim Mathies [:jimm]]

Attachment: fix ContentReceivedTouch v.3

Ok this is working now with one exception, bug 927033 currently breaks block tracking. As soon as we get one touchend in a multitouch set, the touch block apz clears, and things start to fall apart from there.

Comment 29

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 825329 [details] [diff] [review]
metroinput touch point debugging v.1

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

Seems fine to me!

::: widget/windows/winrt/MetroAppShell.cpp
@@ +221,5 @@
>  // queue and need to make sure they get dispatched before the stack unwinds.
>  void // static
>  MetroAppShell::MarkEventQueueForPurge()
>  {
> +  //LogFunction();

Let's just remove these; don't comment them out. Commented-out code just becomes clutter.

@@ +242,5 @@
>    if (!sWillEmptyThreadQueue) {
>      return;
>    }
>  
> +  //LogFunction();

Let's just remove these; don't comment them out. Commented-out code just becomes clutter.

::: widget/windows/winrt/MetroInput.cpp
@@ +1159,5 @@
>      NS_NewRunnableMethod(this, &MetroInput::DeliverNextQueuedTouchEvent);
>    NS_DispatchToCurrentThread(runnable);
>  }
>  
> +static void DumpTouchIds(const char* aMsg, WidgetTouchEvent* aEvent)

I'm a fan of using the unnamed namespace instead of declaring functions |static|. I know that the style guide says we prefer |static| [1], I just disagree. (Don't change this; I just wanted to complain :)

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Anonymous_namespaces

@@ +1183,5 @@
> +  nsTArray< nsRefPtr<dom::Touch> >& touches = aEvent->touches;
> +  for (uint32_t i = 0; i < touches.Length(); ++i) {
> +    dom::Touch* touch = touches[i];
> +    if (!touch) {
> +      continue;

Can this actually happen? Should we warn?

@@ +1186,5 @@
> +    if (!touch) {
> +      continue;
> +    }
> +    int32_t id = touch->Identifier();
> +    WinUtils::Log("   id=%d target=%s", id, aMsg);

Should we rename |aMsg| to something like |aTarget|?

Comment 30

[Jim Mathies [:jimm]]

Attachment: fix ContentReceivedTouch v.3

With matt's patch in bug 927033 this is working well.

Comment 31

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

Comment on attachment 825415 [details] [diff] [review]
fix ContentReceivedTouch v.3

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

A few nits below. But my main concern is that the APZCTreeManager currently has code to redirect input events from from one APZC instance to another in the middle of a touch block, which can leave your touch-track code in an inconsistent state. Specifically, say you have a page with an iframe, there is an APZC for the root document and one for the iframe. If you start panning with one finger on the iframe, and the add a second finger to turn it into a pinch, the panning-related touch events will be directed to the inner APZC but as soon as the second touch point appears the events will be directed to the parent APZC. The inner APZC is never notified of the switch and so will still think there is one finger down; if both fingers are subsequently lifted then the inner APZC will be in a bad state.

I think overall the approach of this patch is fine but it needs to handle this case. The simplest way would be to add another function to clear the state when the APZCTreeManager starts routing events to the parent APZC.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +56,5 @@
>  // #define APZC_LOG(...) printf_stderr("APZC: " __VA_ARGS__)
> +// #include "WinUtils.h"
> +// #define APZC_LOG(...) mozilla::widget::WinUtils::Log("APZC: " __VA_ARGS__)
> +#define APZC_LOG_FM(fm, prefix, ...)
> +#ifndef APZC_LOG_FM

I'd rather leave this the way it was originally unless you have a strong argument against. As we discussed on IRC printf_stderr also goes to the debugger if there is one attached so for most use cases the existing logging code should be fine.

@@ +483,5 @@
> +          APZC_LOG("%p touchblock: *warning* mismatch id=%d\n", this, multiTouchInput.mTouches[idx].mIdentifier);
> +          MOZ_ASSERT("mismatched touch pointer ids in this touch block!");
> +        }
> +        APZC_LOG("%p touchblock: remove id=%d\n", this, multiTouchInput.mTouches[idx].mIdentifier);
> +        mTouchBlockPointIds.RemoveElement(multiTouchInput.mTouches[idx].mIdentifier);

Can you just use the return value from RemoveElement rather than doing a separate call to Contains above?

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +520,5 @@
>    already_AddRefed<GeckoContentController> GetGeckoContentController();
>    already_AddRefed<GestureEventListener> GetGestureEventListener();
>  
> +  bool mDisableNextTouchBlock;
> +  nsTArray<uint32_t> mTouchBlockPointIds;

I think this should be int32_t, since that's what mIdentifier is.

Comment 32

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> I think overall the approach of this patch is fine but it needs to handle
> this case. The simplest way would be to add another function to clear the
> state when the APZCTreeManager starts routing events to the parent APZC.

Thinking about this a bit more, it might be simpler to move this touch block code into APZCTreeManager and build on top of what mbrubeck did in bug 927033.

Comment 33

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> > I think overall the approach of this patch is fine but it needs to handle
> > this case. The simplest way would be to add another function to clear the
> > state when the APZCTreeManager starts routing events to the parent APZC.
> 
> Thinking about this a bit more, it might be simpler to move this touch block
> code into APZCTreeManager and build on top of what mbrubeck did in bug
> 927033.

Ok, I can do that.

I'm concerned though about wrapping here. (My original patch didn't deal with this either really.) What are the chances we'll get a delayed ContentReceivedTouch for a previous touch block on b2g or android?

Seems like the tree manager needs a way to correlate an apz to a touch block, and to track a set of touch ids for touch blocks. Also as you pointed out it also needs to be able to shift all this information from one apz to another in the event that the target apz changes. ContentReceivedTouch also needs to identify the touch block it's associated with. Or maybe this is all overkill?

For metro wrapping isn't an issue, so maybe we could fix ContentReceivedTouch here ignoring wrapping, and file a follow up on revamping how we track state.

Comment 34

[Jim Mathies [:jimm]]

Note I can improve my existing patch ever so slightly here, by checking mTouchBlockPointIds.Length() before setting mDisableThisTouchBlock to true. This way a simple touchdown -> touchup -> ContentReceivedTouch(true) can't kill the subsequent touch block accidentally. But this doesn't protect against a really late ContentReceivedTouch(true) that comes in after we start processing the next block on the input thread.

Comment 35

[Jim Mathies [:jimm]]

Attachment: fix ContentReceivedTouch v.4

This relies on the touch counting we do in bug 927033. If you like I can bring in the pointer id tracking I had in the previous patch.

Comment 36

[Jim Mathies [:jimm]]

I wonder if we should make ContentReceivedTouch an internal tree manager call that it calls after receiving touchcancel for all touch points while the target apzc is in state WAITING_LISTENERS? Since our touch tracking expects touchcancels for all points anyway, it seems silly to expect callers to call both ContentReceivedTouch and send touchcancel events.

The flip side of this would be to keep ContentReceivedTouch as public and require callers to call it, but not require touchcancel events.

Comment 37

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

Comment on attachment 826053 [details] [diff] [review]
fix ContentReceivedTouch v.4

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

Sorry for the late feedback; I've been try to focus as much as I can to figure out the zooming issues and so have been putting everything else off :( I do prefer the approach of this patch to the previous one.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +242,5 @@
>        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
>        if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
>          mTouchCount++;
>          mApzcForInputBlock = GetTargetAPZC(ScreenPoint(multiTouchInput.mTouches[0].mScreenPoint));
> +        nsRefPtr<AsyncPanZoomController> oldBlockApzc = mApzcForInputBlock;

This part isn't right, I think. Consider the case where you have a page with an iframe. You put your finger down on the iframe and start panning. At this point mApzcForInputBlock is the APZC for the iframe, and all events are getting directed straight there. Then, while your finger is still down, you drag it outside the bounds of the iframe. mApzcForInputBlock is still the iframe, and events are still going there. Now you put down a second finger (also outside the iframe) to pinch. At this point this MULTITOUCH_START code will run, GetTargetAPZC(mTouches[0]) will return the top-level page, and that's what you're going to set as your oldBlockApzc, when really you want the oldBlockApzc to be the APZC for the iframe.

@@ +256,5 @@
> +          // it's not going to be receiving any additional input for this touch
> +          // block.
> +          if (oldBlockApzc && oldBlockApzc != mApzcForInputBlock) {
> +            oldBlockApzc->CancelTouchBlock();
> +          }

I think you can move this if condition outside the for loop. No events are actually sent to the APZC inside the loop so there's no need to cancel touch blocks on them.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +54,5 @@
>  
>  #define APZC_LOG(...)
>  // #define APZC_LOG(...) printf_stderr("APZC: " __VA_ARGS__)
> +#define APZC_LOG_FM(fm, prefix, ...)
> +#ifndef APZC_LOG_FM

I'd prefer if you just left this as-is. Having the framemetrics logging on by default is quite useful.

Comment 38

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

(In reply to Jim Mathies [:jimm] from comment #33)
> I'm concerned though about wrapping here. (My original patch didn't deal
> with this either really.) What are the chances we'll get a delayed
> ContentReceivedTouch for a previous touch block on b2g or android?

It is quite possible for this to happen on android at least, because of the additional UI thread that operates separately from the gecko thread. Lags in gecko can easily result in the wrapping scenario. Less likely on b2g and (for now) metro but still possible. It would be good to come up with a solution that handles the wrapping scenario as well.

> Seems like the tree manager needs a way to correlate an apz to a touch
> block, and to track a set of touch ids for touch blocks. Also as you pointed
> out it also needs to be able to shift all this information from one apz to
> another in the event that the target apz changes. ContentReceivedTouch also
> needs to identify the touch block it's associated with. Or maybe this is all
> overkill?

I think you can get by without some of this. It would be worth reading through the documentation in mobile/android/base/gfx/TouchEventHandler.java to see how I solved this problem there. Some elements of that can be reused here, appropriately modified for the multi-apzc cases.

> For metro wrapping isn't an issue, so maybe we could fix
> ContentReceivedTouch here ignoring wrapping, and file a follow up on
> revamping how we track state.

I'm ok with doing this as a fallback but I'd like to spend a bit of time trying to see if we can at least come up with a plan on how to solve the wrapping problem.

Comment 39

[Jim Mathies [:jimm]]

Attachment: part 1 - return the guid of the target apzc v.1

Comment 40

[Jim Mathies [:jimm]]

Attachment: part 2 - metro widget guid changes v.1

Comment 41

[Jim Mathies [:jimm]]

Attachment: part 3 - port balancing logic v.1

Comment 42

[Jim Mathies [:jimm]]

Attachment: part 1 - return the guid of the target apzc v.1

Comment 43

[Jim Mathies [:jimm]]

Attachment: part 2 - metro widget guid changes v.1

Comment 44

[Jim Mathies [:jimm]]

Attachment: part 3 - port balancing logic v.1

Comment 45

[Jim Mathies [:jimm]]

Attachment: part 4 - deal with apz target changes in tree manager v.1

Comment 46

[Jim Mathies [:jimm]]

Attachment: part 5 - debugging output (optional)

Comment 47

[Jim Mathies [:jimm]]

Comment on attachment 828737 [details] [diff] [review]
part 1 - return the guid of the target apzc v.1

matt's going to land bug 927033 shortly so these should be good to go.

Comment 48

[Jim Mathies [:jimm]]

Comment on attachment 828738 [details] [diff] [review]
part 2 - metro widget guid changes v.1

Metro widget part of tracking the guid plus some refactoring. The core change is in MetroInput where we call ApzReceiveInputEvent retrieving the guid, and then hand that back in calls to ApzContentConsumingTouch/ApzContentIgnoringTouch.

The refactoring is a move toward having a non-static tree manager object - we plan to support multiple widgets next year.

Comment 49

[Jim Mathies [:jimm]]

Attachment: part 3 - port balancing logic v.1

Comment 50

[Jim Mathies [:jimm]]

Attachment: part 4 - deal with apz target changes in tree manager v.1

Comment 51

[Jim Mathies [:jimm]]

Attachment: part 5 - touchstart fix for metro input v.1

Comment 52

[Jim Mathies [:jimm]]

Attachment: part 6 - logging (optional)

Comment 53

[Jim Mathies [:jimm]]

As soon as I finish up a build, I'll post patches merged to tip.

Comment 54

[Jim Mathies [:jimm]]

Attachment: part 1 - return the guid of the target apzc v.1

Comment 55

[Jim Mathies [:jimm]]

Attachment: part 2 - metro widget guid changes v.1

matt mind reviewing this one? it's widget related. See comment 48 for a description.

Comment 56

[Jim Mathies [:jimm]]

Attachment: part 3 - port balancing logic v.1

Comment 57

[Jim Mathies [:jimm]]

Attachment: part 4 - deal with apz target changes in tree manager v.1

Comment 58

[Jim Mathies [:jimm]]

Attachment: part 3 - touchstart fix for metro input v.1

Comment 59

[Jim Mathies [:jimm]]

Attachment: part 6 - logging (optional)

Comment 60

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

Comment on attachment 829294 [details] [diff] [review]
part 1 - return the guid of the target apzc v.1

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

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +224,5 @@
>     */
>    nsEventStatus HandleInputEvent(const InputData& aEvent);
>  
>    /**
> +   * Returns the scrollable guid of this apzc.

Change this to "Populates the provided object with the scrollable guid of this apzc". Maybe also rename the variable to aOutGuid for consistency.

Comment 61

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #60)
> Comment on attachment 829294 [details] [diff] [review]
> part 1 - return the guid of the target apzc v.1
> 
> Review of attachment 829294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/AsyncPanZoomController.h
> @@ +224,5 @@
> >     */
> >    nsEventStatus HandleInputEvent(const InputData& aEvent);
> >  
> >    /**
> > +   * Returns the scrollable guid of this apzc.
> 
> Change this to "Populates the provided object with the scrollable guid of
> this apzc". Maybe also rename the variable to aOutGuid for consistency.

Or, to be even more consistent, aGuidOut (like e.g. aTransformToApzcOut [1]).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#744

Comment 62

[Jim Mathies [:jimm]]

Comment on attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1

up next..

Comment 63

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

Comment on attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1

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

So for the most part this looks good. One problem is that by itself this change will probably have unexpected effects on B2G, because B2G is also going to end up using this code (and incorrectly). So we'll need to fix that up before we can land this. The other thing is that code assumes the metro definition of a touch block, where the block extends from the first touch down to the last touch up. While that would be fine if this were just on Metro, it means we'll have to use the same definition on B2G. As discussed during our meeting today, I'm not sure if it's better to use this definition on both B2G and Metro now and then change them both later, or just change it now in Metro and then use the right one going forward.

Comment 64

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

Comment on attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1

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

Minusing for now, some comments below.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +325,5 @@
> +  // Clear any pending touch block timeouts
> +  while (!mTouchListenerTimeoutTasks.IsEmpty()) {
> +    CancelableTask* task = mTouchListenerTimeoutTasks[0];
> +    task->Cancel();
> +    mTouchListenerTimeoutTasks.RemoveElementAt(0);

This code will run on the compositor thread, so access to mTouchListenerTimeoutTasks needs to be thread-safe.

@@ +375,5 @@
>      const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
>      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> +      // Only set this state if we are on the start of a new touch block.
> +      if (queueHasClosedBlock) {
> +        SetState(WAITING_LISTENERS);

I think there's a scenario here that falls through the cracks:

Say you put a finger down and move it around some. APZC goes into the WAITING_LISTENERS state and builds up a queue of touches. Then you get a ContentReceivedTouch(false) and so ProcessTouchBlock runs and clears out the queue. As part of the processing of the touch block mState ends up in state TOUCHING or PANNING or something.

Now you put a second finger down. In this case TouchQueueHasClosedBlock() will return true because the queue is empty, all of the conditions here will pass, and we'll set state WAITING_LISTENERS and queue up a TimeoutTouchListeners call.

However, because this is a second-finger-goes-down touch block, it is inconsistent with Metro's definition of a touch block, and so Metro will never inform the APZC of the ContentReceivedTouch status of this touch block. This will throw the balance out of whack. Or am I missing something?

@@ +387,5 @@
> +      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> +        // Only fire off a new TimeoutTouchListener for the first touchstart in
> +        // a touch block.
> +        if (queueHasClosedBlock) {
> +          CancelableTask* task = 

nit: trailing whitespace

@@ +1428,5 @@
>  }
>  
> +bool AsyncPanZoomController::TouchQueueHasClosedBlock() {
> +  MonitorAutoLock lock(mTouchQueueMonitor);
> +  int32_t balanceCount = 0;

s/balanceCount/pointerCount/ ?

@@ +1498,1 @@
>    if (!mFrameMetrics.mMayHaveTouchListeners) {

Eventually we'll have to deal with mMayHaveTouchListeners changing but right now I think that never happens (another bug)

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +738,5 @@
> +   /* An array of touch timeouts that have been sent */
> +  nsTArray<CancelableTask*> mTouchListenerTimeoutTasks;
> +   /* The "processing balance" value */
> +  int32_t mProcessingBalance;
> +   /* mTouchQueue monitor */

This comment isn't much use. Might as well remove it, but update the comments of the other variables to indicate that they are protected by this monitor.

Comment 65

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

Comment on attachment 829300 [details] [diff] [review]
part 4 - deal with apz target changes in tree manager v.1

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1462,5 @@
> +  MonitorAutoLock lock(mTouchQueueMonitor);
> +  mTouchQueue.Clear();
> +  // We can't cancel ContentReceivedTouch calls, so save the
> +  // number of calls we are expecting so we can ignore them.
> +  mIgnoreContentReceivedTouchCount = mContentReceivedTouchCount;

I don't know if we need to do this or the call to ClearTouchTimers() above. The processing balance already tracks what's outstanding. As long as the mTouchQueue is cleared we should be fine with this APZC receiving whatever ContentReceivedTouch and TimeoutTouchListeners are outstanding, because they will effectively be no-ops on an empty touch queue.

Comment 66

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

Comment on attachment 825329 [details] [diff] [review]
metroinput touch point debugging v.1

In other components we set the checkin+ flag on landed patches in multi-patch bugs. Hope you don't mind if we do the same here.

Comment 67

[Jim Mathies [:jimm]]

Comment on attachment 829299 [details] [diff] [review]
part 3 - port balancing logic v.1

Not going to do this now since B2G is totally incompatible with it.

Comment 68

[Jim Mathies [:jimm]]

Comment on attachment 829300 [details] [diff] [review]
part 4 - deal with apz target changes in tree manager v.1

If we still need some part of this I'll resurrect it.

Comment 69

[Jim Mathies [:jimm]]

Attachment: part 4 - add a transform helper to tree manager

Last part of this that we need here. Per your suggestion I skipped the transform caching and did basically the same thing we were doing with mouse events before. Works pretty well on a canvas drawing test case I have which I'll post.

Comment 70

[Jim Mathies [:jimm]]

Attachment: cavas drawing test case

Comment 71

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

Comment on attachment 829546 [details] [diff] [review]
part 4 - add a transform helper to tree manager

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

r=me with the change described below.

::: gfx/layers/composite/APZCTreeManager.h
@@ +194,5 @@
> +   * A helper for transforming coordinates to gecko coordinate space.
> +   *
> +   * @param aPoint point to transform
> +   */
> +  void TransformCoordinateToGecko(LayoutDeviceIntPoint& aPoint);

I'd prefer if this function took a const ScreenIntPoint as input and returned a LayoutDeviceIntPoint as output. That will match the actual operation that is happening. Eventually I want input events to have both as well rather than doing the in-place modification with confusing coordinate systems like they do now.

Comment 72

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #71)
> Comment on attachment 829546 [details] [diff] [review]
> part 4 - add a transform helper to tree manager
> 
> Review of attachment 829546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the change described below.
> 
> ::: gfx/layers/composite/APZCTreeManager.h
> @@ +194,5 @@
> > +   * A helper for transforming coordinates to gecko coordinate space.
> > +   *
> > +   * @param aPoint point to transform
> > +   */
> > +  void TransformCoordinateToGecko(LayoutDeviceIntPoint& aPoint);
> 
> I'd prefer if this function took a const ScreenIntPoint as input and
> returned a LayoutDeviceIntPoint as output. That will match the actual
> operation that is happening. Eventually I want input events to have both as
> well rather than doing the in-place modification with confusing coordinate
> systems like they do now.

Ok, no problem will update.

I pushed everything to try, will wait for the results there and if green, I'll push this over the weekend.

Comment 73

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de4707d94a3c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/43062f25affd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbbb289f73c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6c3bc7c250

Comment 74

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de4707d94a3c
https://hg.mozilla.org/mozilla-central/rev/43062f25affd
https://hg.mozilla.org/mozilla-central/rev/7dbbb289f73c
https://hg.mozilla.org/mozilla-central/rev/bf6c3bc7c250