Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 1 • 6 years ago

Btw. we should also inspect other commands for Promise handling just to make sure we don't have similar behavior somewhere else.

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 6 years ago
mozreview-review

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 6 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 6 • 6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8cc844125d5
[marionette] Handle errors for nested promise in dispatchPointerMove. r=ato

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 6 years ago
mozreview-review-reply

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?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 6 years ago
mozreview-review-reply

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.

	Cristian Brindusan [:cbrindusan]
	Comment 9 • 6 years ago
bugherder

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 6 years ago

This test-only patch fixes a possible hang. We want this to be fixed in the next Firefox release. Please uplift to beta.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/f8c51a291615

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 6 years ago

To not have our testers experience a hang please also uplift to esr60. Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/606b6857c9a4