dispatchPointerMove can hang if exceptions are thrown within performOnePointerMove

RESOLVED FIXED in Firefox -esr60

Status

defect
P1
critical
RESOLVED FIXED
Last year
Last year

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

({hang})

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox-esr60 fixed, firefox61 fixed, firefox62 fixed)

Details

Attachments

(1 attachment)

There is a chance of seeing hangs in `dispatchPointerMove` especially then when any code inside the Promises is raising an exception like in performOnePointerMove.

We should rework the Promise handling to ensure that the error gets correctly propagated, and returned as response to the driver.
Btw. we should also inspect other commands for Promise handling just to make sure we don't have similar behavior somewhere else.
Comment on attachment 8984410 [details]
Bug 1467743 - [marionette] Handle errors for nested promise in dispatchPointerMove.

https://reviewboard.mozilla.org/r/250246/#review256556

::: commit-message-e0595:4
(Diff revision 1)
> +Bug 1467743 - [marionette] Handle exceptions for inner promise in dispatchPointerMove.
> +
> +Because the inner promise doesn't use the "catch()" method a
> +possible raised exception by its code is not handled, and will

s/exception/error/

::: testing/marionette/action.js:1370
(Diff revision 1)
>      intermediatePointerEvents.then(() => {
>        performOnePointerMove(inputState, targetX, targetY, window);
>        resolve();
> +    }).catch(err => {
> +      reject(err);
>      });
> -

Do you really need to use catch() here?  I thought it was the case
that any errors would automatically get propagated and the promise
rejected?

Possibly they are not if the error is thrown inside a then()?  If
this is the case you can remove the then() by using await on
intermediatePointerEvents, which *should* save you from having to
cvatch the error and reject.
Attachment #8984410 - Flags: review?(ato) → review+
Comment on attachment 8984410 [details]
Bug 1467743 - [marionette] Handle errors for nested promise in dispatchPointerMove.

https://reviewboard.mozilla.org/r/250246/#review256556

> Do you really need to use catch() here?  I thought it was the case
> that any errors would automatically get propagated and the promise
> rejected?
> 
> Possibly they are not if the error is thrown inside a then()?  If
> this is the case you can remove the then() by using await on
> intermediatePointerEvents, which *should* save you from having to
> cvatch the error and reject.

No, this only works when you use `await`, which is not the case here. Also this is a nested promise, which means the error has to be propagated to the outer promise.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8cc844125d5
[marionette] Handle errors for nested promise in dispatchPointerMove. r=ato
Comment on attachment 8984410 [details]
Bug 1467743 - [marionette] Handle errors for nested promise in dispatchPointerMove.

https://reviewboard.mozilla.org/r/250246/#review256556

> No, this only works when you use `await`, which is not the case here. Also this is a nested promise, which means the error has to be propagated to the outer promise.

And what’s the reason we can’t await?
Comment on attachment 8984410 [details]
Bug 1467743 - [marionette] Handle errors for nested promise in dispatchPointerMove.

https://reviewboard.mozilla.org/r/250246/#review256556

> And what’s the reason we can’t await?

The bigger fish to fry would be to get rid of the nested promise, but in this case I just wanted to fix the hang.
https://hg.mozilla.org/mozilla-central/rev/e8cc844125d5
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
This test-only patch fixes a possible hang. We want this to be fixed in the next Firefox release. Please uplift to beta.
Whiteboard: [checkin-needed-beta]
To not have our testers experience a hang please also uplift to esr60. Thanks.
Whiteboard: [checkin-needed-esr60]
You need to log in before you can comment on or make changes to this bug.