985541 - Turn GestureEventListener into finite-state machine

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[dmitry.rojkov]

Attachment: Turn GestureEventListener into a finite-state machine

Currently it's not an easy task to debug APZ issues when events travel from APZC to GestureEventListener forth and back. Additionally the GestureEventListener class has evolved so that it's difficult to reason about the state of its instance.

I'd like to propose a new implementation of GestureEventListener where states are clearly defined and state transitions happen in more controlled way. Basically the implementation represents a sort of a Mealy machine where the alphabet of input signals consists of argument-less methods and all the state transitions are calculated inside that functions.

Here is the try build for the proposed patch
https://tbpl.mozilla.org/?tree=Try&rev=2108a0df0a1a

Note: There's an issue in TestAsyncPanZoomController.cpp - the mock object MockContentControllerDelayed does assumptions on how many CancelableTask's can be created at once through PostDelayedTask(). I had to remove this assertion to make all the tests pass. I'm not sure what would be a better solution for it.

Comment 1

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

Comment on attachment 8393590 [details] [diff] [review]
Turn GestureEventListener into a finite-state machine

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

Overall I really really like this change, it makes GEL much easier to follow! I have pointed out some small issues below.

::: gfx/layers/ipc/GestureEventListener.cpp
@@ +33,5 @@
>  
> +void GetCurrentSpanAndFocus(const MultiTouchInput& aEvent, float& aOutCurrentSpan, ScreenPoint& aOutFocusPoint)
> +{
> +    const ScreenIntPoint& firstTouch = aEvent.mTouches[0].mScreenPoint,
> +                         secondTouch = aEvent.mTouches[1].mScreenPoint;

2-space indent this function

@@ +43,5 @@
> +
> +float GetCurrentSpan(const MultiTouchInput& aEvent)
> +{
> +    const ScreenIntPoint& firstTouch = aEvent.mTouches[0].mScreenPoint,
> +                         secondTouch = aEvent.mTouches[1].mScreenPoint;

2-space indent this function

@@ +117,5 @@
> +    SetState(GESTURE_FIRST_SINGLE_TOUCH_DOWN);
> +    mTouchStartPosition = mLastTouchInput.mTouches[0].mScreenPoint;
> +
> +    mLongTapTimeoutTask =
> +      NewRunnableMethod(this, &GestureEventListener::HandleInputTimeoutLongTap);

Split this into a CreateLongTapTimeoutTask the same way you did it for CreateMaxTapTimeoutTask

@@ +131,5 @@
> +    break;
> +  default:
> +    NS_WARNING("Unhandled state!");
> +    SetState(GESTURE_NONE);
> +  }

Add a break; statement at the end of the default case, here and in all other functions below as well

@@ +139,2 @@
>  {
> +  switch (mState) {

It's also possible to enter this function in states GESTURE_FIRST_SINGLE_TOUCH_UP, GESTURE_LONG_TOUCH_DOWN, GESTURE_MULTI_TOUCH_DOWN, and GESTURE_PINCH. Those aren't handled.

@@ +177,5 @@
> +    // If we move too much, bail out of the tap.
> +    ScreenIntPoint delta = mLastTouchInput.mTouches[0].mScreenPoint - mTouchStartPosition;
> +    if (NS_hypot(delta.x, delta.y) > AsyncPanZoomController::GetTouchStartTolerance()) {
> +      CancelLongTapTimeoutTask();
> +      if (mState != GESTURE_FIRST_SINGLE_TOUCH_MAX_TAP_DOWN) CancelMaxTapTimeoutTask();

Please always use braces for if statements, even if they're one-liners. And in this case (assuming you fix the other review comments below) you should be able to call CancelMaxTimeoutTask without the condition and it should just be a no-op if mState == GESTURE_FIRST_SINGLE_TOUCH_MAX_TAP_DOWN

@@ +332,4 @@
>  
> +void GestureEventListener::HandleInputTimeoutLongTap()
> +{
> +  if (mState == GESTURE_FIRST_SINGLE_TOUCH_MAX_TAP_DOWN ||

This function should null out mLongTapTimeoutTask to avoid keeping a dangling pointer to a deleted thing.

@@ +347,5 @@
>    }
>  }
>  
> +void GestureEventListener::HandleInputTimeoutMaxTap()
> +{

This function should null out the mMaxTapTimeoutTask to prevent keeping a dangling pointer.

@@ +365,2 @@
>  {
> +    TapGestureInput tapEvent(TapGestureInput::TAPGESTURE_CONFIRMED,

2-space indent this function

::: gfx/layers/ipc/GestureEventListener.h
@@ +161,5 @@
>     * we can cancel it if any other touch event happens.
>     * CancelLongTapTimeoutTask: Cancel the mLongTapTimeoutTask and also set
>     * it to null.
>     */
>    CancelableTask *mLongTapTimeoutTask;

Update the comment to also mention what states are allowed to have mLongTapTimeoutTask as non-null

@@ +170,2 @@
>     */
> +  CancelableTask *mMaxTapTimeoutTask;

Ditto here; mention what states are allowed to have mMaxTapTimeoutTask as non-null

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +69,5 @@
>    {
>    }
>  
>    void PostDelayedTask(Task* aTask, int aDelayMs) {
> +    if (mCurrentTask != nullptr) return;

We'll need to update this mock object to handle multiple concurrent tasks somehow. I think it would be best to replace mCurrentTask with a queue of tasks, and make the RunDelayedTask function run the next one in the queue. We might need to change CheckHasDelayedTask and ClearDelayedTask as well - I don't know enough about what tests were failing to suggest what to change there.

Comment 2

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8393590 [details] [diff] [review]
Turn GestureEventListener into a finite-state machine

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

Thanks for your work on this! I agree with kats and have some comments of my own.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +597,5 @@
> +  mLastEventTime = aEvent.mTime;
> +  return rv;
> +}
> +
> +nsEventStatus AsyncPanZoomController::HandleGestureEvent(const InputData& aEvent)

I don't see a reason to separate this into another function, and I think APZC should be agnostic to the type of input it gets until it checks mType. I don't care a lot about this, though.

@@ +628,5 @@
>    }
>    default: NS_WARNING("Unhandled input event"); break;
>    }
>  
> +  mLastEventTime = aEvent.mTime; // FIXME: is this needed?

I don't think keeping this will hurt for consistency in case we ever need it for gesture timing calculations, but if we re-combine HandleInputEvent and HandleGestureEvent we'll go back to the previous functionality anyways.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +255,5 @@
>     */
>    nsEventStatus HandleInputEvent(const InputData& aEvent);
>  
>    /**
> +   * Handler for gesture events comming from GestureEventListener.

If you do keep this, I think we should be careful and not say unnecessarily explicit things like this. Gesture events are not inherently tied to GEL, but they happen to come entirely from there right now.

::: gfx/layers/ipc/GestureEventListener.cpp
@@ +30,5 @@
>   * distance, not a displacement.
>   */
>  static const float PINCH_START_THRESHOLD = 35.0f;
>  
> +void GetCurrentSpanAndFocus(const MultiTouchInput& aEvent, float& aOutCurrentSpan, ScreenPoint& aOutFocusPoint)

It's hard to fit the contents of a lot of lines related to APZC into 80 characters and we've been pretty lazy about that so far, but this seems like a pretty ripe candidate for being broken up a bit.

@@ +40,5 @@
> +    ScreenIntPoint delta = secondTouch - firstTouch;
> +    aOutCurrentSpan = float(NS_hypot(delta.x, delta.y));
> +}
> +
> +float GetCurrentSpan(const MultiTouchInput& aEvent)

Please you make these two functions GetCurrentFocus and GetCurrentSpan instead of GetCurrentSpanAndFocus and GetCurrentSpan. That way, GetCurrentSpanAndFocus isn't just repeating GetCurrentSpan with some added functionality.

@@ +85,5 @@
> +    }
> +    break;
> +  case MultiTouchInput::MULTITOUCH_MOVE:
> +    // TODO: can we drop return value of HandleInputTouchMove()?
> +    rv = HandleInputTouchMove();

I think we should generally use this form whenever possible. The HandleInput*() methods are basically fallthroughs from HandleInputEvent(), so HandleInputEvent() should return their return values. Not doing this, or not being able to, is a symptom of a design problem. Since we're basically rewriting the whole thing, we'd might as well solve these problems now.

@@ +168,5 @@
> +  switch (mState) {
> +  case GESTURE_NONE:
> +  case GESTURE_LONG_TOUCH_DOWN:
> +    // Ignore this input signal while in GESTURE_NONE and
> +    // GESTURE_LONG_TOUCH_DOWN as the corresponding events get handled by APZC.

The comment doesn't need to include the states you're referring to, since they're directly above the comment.

@@ +185,5 @@
>    }
>  
> +  case GESTURE_MULTI_TOUCH_DOWN: {
> +    if (mLastTouchInput.mTouches.Length() < 2) {
> +      NS_WARNING("Wrong input: less than 2 moving points in GESTURE_MULTI_TOUCH_DOWN state!\n");

You don't need to provide a line break to NS_WARNING. Please make this change everywhere.

@@ +194,5 @@
> +    ScreenPoint focusPoint;
> +    GetCurrentSpanAndFocus(mLastTouchInput, currentSpan, focusPoint);
> +
> +    mSpanChange += fabsf(currentSpan - mPreviousSpan);
> +    if (mSpanChange > PINCH_START_THRESHOLD) {

I believe I could avoid entering a pinch gesture here by pinching very slowly. This wasn't a problem originally because we weren't updating mPreviousSpan each time this path is executed (oops?).

::: gfx/layers/ipc/GestureEventListener.h
@@ -36,5 @@
> - * and AsyncPanZoomController is expected to handle it.
> - *
> - * Android doesn't use this class because it has its own built-in gesture event
> - * listeners that should generally be preferred.
> - */

Why did you remove this comment? None of it has changed, though it could use some tweaking in certain spots.

@@ +47,5 @@
> +  enum GestureState {
> +    // This is the initial and final state of any gesture.
> +    // In this state there's no gesture going on, and we don't think we're
> +    // about to enter one.
> +    // Allowed next states: GESTURE_FIRST_SINGLE_TOUCH_DOWN, GESTURE_MULTI_TOUCH_DOWN.

Nice! I really like how you included the allowed next states.

@@ +67,5 @@
> +    //                      GESTURE_LONG_TOUCH_DOWN.
> +    GESTURE_FIRST_SINGLE_TOUCH_MAX_TAP_DOWN,
> +
> +    // A user put her finger down and lifted it up quickly enough.
> +    // After having gotten into this state we start MAX_TAP_TIME timer again.

This comment is slightly unclear. "After having gotten into this state, we clear the timer for MAX_TAP_TIMER." maybe?

@@ +150,5 @@
>    MultiTouchInput mLastTouchInput;
>  
>    /**
> +   * Position of the last touch starting. This is only valid during an attempt
> +   * to determine if a touch is a tap.

Please be a bit more specific here. Include the state that it's used in, maybe.

@@ +170,4 @@
>     */
> +  CancelableTask *mMaxTapTimeoutTask;
> +  inline void CancelMaxTapTimeoutTask();
> +  inline void CreateMaxTapTimeoutTask();

Is there a specific reason you inlined these? They may be small, but I'd rather let the compiler figure out what to do here.

Comment 3

[dmitry.rojkov]

Thanks for your feedback! I'll address your comments on Monday.

Comment 4

[dmitry.rojkov]

Attachment: Turn GestureEventListener into a finite-state machine

Thanks again guys for very valuable feedback. This is the second version addressing the most of your comments.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)

> ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
> @@ +69,5 @@
> >    {
> >    }
> >  
> >    void PostDelayedTask(Task* aTask, int aDelayMs) {
> > +    if (mCurrentTask != nullptr) return;
> 
> We'll need to update this mock object to handle multiple concurrent tasks
> somehow. I think it would be best to replace mCurrentTask with a queue of
> tasks, and make the RunDelayedTask function run the next one in the queue.
> We might need to change CheckHasDelayedTask and ClearDelayedTask as well - I
> don't know enough about what tests were failing to suggest what to change
> there.

Basically all the tests emulating a tap were failing because at the moment of "tap down" there two cancelable tasks were created (https://tbpl.mozilla.org/?tree=Try&rev=4d2ff21e0211):
TEST-UNEXPECTED-FAIL | AsyncPanZoomControllerTester.ShortPress | Value of: nullptr == mCurrentTask
TEST-UNEXPECTED-FAIL | AsyncPanZoomControllerTester.ShortPress | test completed (time: 0ms)
TEST-UNEXPECTED-FAIL | AsyncPanZoomControllerTester.MediumPress | Value of: nullptr == mCurrentTask
TEST-UNEXPECTED-FAIL | AsyncPanZoomControllerTester.MediumPress | test completed (time: 0ms)
TEST-UNEXPECTED-FAIL | AsyncPanZoomControllerTester.LongPressPreventDefault | Value of: nullptr == mCurrentTask

I'll try to replace mCurrentTask with a queue in the part2.

(In reply to Doug Sherk (:drs) from comment #2)

> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> @@ +597,5 @@
> > +  mLastEventTime = aEvent.mTime;
> > +  return rv;
> > +}
> > +
> > +nsEventStatus AsyncPanZoomController::HandleGestureEvent(const InputData& aEvent)
> 
> I don't see a reason to separate this into another function, and I think
> APZC should be agnostic to the type of input it gets until it checks mType.
> I don't care a lot about this, though.

My main concern was that AsyncPanZoomController::HandleInputEvent() and GestureEventListener::HandleInputEvent() were 1) mutually recursive 2) changing internal state of each other. That smelled not well to me.
 
> @@ +194,5 @@
> > +    ScreenPoint focusPoint;
> > +    GetCurrentSpanAndFocus(mLastTouchInput, currentSpan, focusPoint);
> > +
> > +    mSpanChange += fabsf(currentSpan - mPreviousSpan);
> > +    if (mSpanChange > PINCH_START_THRESHOLD) {
> 
> I believe I could avoid entering a pinch gesture here by pinching very
> slowly. This wasn't a problem originally because we weren't updating
> mPreviousSpan each time this path is executed (oops?).

Hm... I didn't get it. Are you sure mPreviousSpan wasn't updated after
every multi touch event?

The try build for this patch:
https://tbpl.mozilla.org/?tree=Try&rev=a15d29ddd577

Comment 5

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

Comment on attachment 8395772 [details] [diff] [review]
Turn GestureEventListener into a finite-state machine

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

Much better, thanks. Have a question about the thread safety, see below.

::: gfx/layers/ipc/GestureEventListener.cpp
@@ +140,5 @@
> +    CancelLongTapTimeoutTask();
> +    CancelMaxTapTimeoutTask();
> +    SetState(GESTURE_MULTI_TOUCH_DOWN);
> +    // Prevent APZC::OnTouchStart() from handling MULTITOUCH_START event
> +    rv= nsEventStatus_eConsumeNoDefault;

nit: space before '=', here and in the below case statements as well

@@ +409,5 @@
> +void GestureEventListener::CancelLongTapTimeoutTask()
> +{
> +  if (mState == GESTURE_SECOND_SINGLE_TOUCH_DOWN) {
> +    // being in this state means the task has been canceled already
> +    return;

What is the threading model here? Will CancelLongTapTimeoutTask always get called on the same thread as HandleInputTimeoutLongTap? If not then we need to add some locking because this is thread-unsafe. For example, if HandleInputTimeoutLongTap gets invoked just after the "if (mLongTapTimeoutTask)" check in this function, then mLongTapTimeoutTask might be set to null right before we call ->Cancel() on it, which will segfault.

Comment 6

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

Comment on attachment 8395772 [details] [diff] [review]
Turn GestureEventListener into a finite-state machine

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

Dropping review flag for now pending response to above questions.

Comment 7

[dmitry.rojkov]

Attachment: Turn GestureEventListener into a finite-state machine

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> 
> nit: space before '=', here and in the below case statements as well

fixed
 
> @@ +409,5 @@
> > +void GestureEventListener::CancelLongTapTimeoutTask()
> > +{
> > +  if (mState == GESTURE_SECOND_SINGLE_TOUCH_DOWN) {
> > +    // being in this state means the task has been canceled already
> > +    return;
> 
> What is the threading model here? Will CancelLongTapTimeoutTask always get
> called on the same thread as HandleInputTimeoutLongTap? If not then we need
> to add some locking because this is thread-unsafe. For example, if
> HandleInputTimeoutLongTap gets invoked just after the "if
> (mLongTapTimeoutTask)" check in this function, then mLongTapTimeoutTask
> might be set to null right before we call ->Cancel() on it, which will
> segfault.

There is no need for locking because APZC::PostDelayedTask() schedules runnables to run on the UI thread which is the thread where GEL lives. I double checked it by printing PlatformThread::CurrentId() from CancelLongTapTimeoutTask() and HandleInputTimeoutLongTap().

Also I've updated the unit tests to use a task queue inside MockContentControllerDelayed instead of mCurrentTask.
With the updated tests I managed to catch a minor bug in GestureEventListener::HandleInputTimeoutLongTap(): missing cancellation of the max_tap timer in the corner case when MAX_TAP_TIME > ContextMenuDelay.

The try build:
https://tbpl.mozilla.org/?tree=Try&rev=bcfa9b96a5e7

Comment 8

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

Comment on attachment 8396657 [details] [diff] [review]
Turn GestureEventListener into a finite-state machine

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +324,5 @@
>    nsEventStatus status = ApzcDown(apzc, aX, aY, aTime);
>    if (mcc != nullptr) {
> +    // There will be delayed tasks posted for the long-tap and MAX_TAP timeouts, but
> +    // if we were provided a non-null mcc we want to clear them.
> +    mcc->ClearDelayedTask();

For consistency, add another CheckHasDelayedTask() call above this

Comment 9

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

(In reply to dmitry.rojkov from comment #7)
> > What is the threading model here? Will CancelLongTapTimeoutTask always get
> > called on the same thread as HandleInputTimeoutLongTap? If not then we need
> > to add some locking because this is thread-unsafe. For example, if
> > HandleInputTimeoutLongTap gets invoked just after the "if
> > (mLongTapTimeoutTask)" check in this function, then mLongTapTimeoutTask
> > might be set to null right before we call ->Cancel() on it, which will
> > segfault.
> 
> There is no need for locking because APZC::PostDelayedTask() schedules
> runnables to run on the UI thread which is the thread where GEL lives. I
> double checked it by printing PlatformThread::CurrentId() from
> CancelLongTapTimeoutTask() and HandleInputTimeoutLongTap().

Ok, I guess this is fine for now on B2G. Eventually the input events will come from a different thread and we'll need to deal with that. But we'll have to audit the rest of the code too so we can just do it then.

> Also I've updated the unit tests to use a task queue inside
> MockContentControllerDelayed instead of mCurrentTask.
> With the updated tests I managed to catch a minor bug in
> GestureEventListener::HandleInputTimeoutLongTap(): missing cancellation of
> the max_tap timer in the corner case when MAX_TAP_TIME > ContextMenuDelay.

Looks good, thanks!

Comment 10

[dmitry.rojkov]

Attachment: Turn GestureEventListener into a finite-state machine

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
> @@ +324,5 @@
> >    nsEventStatus status = ApzcDown(apzc, aX, aY, aTime);
> >    if (mcc != nullptr) {
> > +    // There will be delayed tasks posted for the long-tap and MAX_TAP timeouts, but
> > +    // if we were provided a non-null mcc we want to clear them.
> > +    mcc->ClearDelayedTask();
> 
> For consistency, add another CheckHasDelayedTask() call above this

Yep, done.

The try build for the change:
https://tbpl.mozilla.org/?tree=Try&rev=d2ccdaa7ec70

Comment 11

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

Cool. Whenever you're ready to land let me know or put the checkin-needed keyword on this bug.

Comment 12

[dmitry.rojkov]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Cool. Whenever you're ready to land let me know or put the checkin-needed
> keyword on this bug.

Ah, right. I forgot to add the keyword. :)

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/de2d3a740e06

Comment 14

[Doug Sherk (:drs) (inactive)]

\o/ Thanks for your work on this, Dmitry.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/de2d3a740e06