Closed
Bug 1467743
Opened 7 years ago
Closed 7 years ago
dispatchPointerMove can hang if exceptions are thrown within performOnePointerMove
Categories
(Remote Protocol :: Marionette, defect, P1)
Remote Protocol
Marionette
Tracking
(firefox-esr60 fixed, firefox61 fixed, firefox62 fixed)
RESOLVED
FIXED
mozilla62
People
(Reporter: whimboo, Assigned: whimboo)
Details
(Keywords: hang)
Attachments
(1 file)
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.
Assignee | ||
Comment 1•7 years ago
|
||
Btw. we should also inspect other commands for Promise handling just to make sure we don't have similar behavior somewhere else.
Comment hidden (mozreview-request) |
Comment 3•7 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.
Attachment #8984410 -
Flags: review?(ato) → review+
Assignee | ||
Comment 4•7 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.
Comment hidden (mozreview-request) |
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8cc844125d5
[marionette] Handle errors for nested promise in dispatchPointerMove. r=ato
Comment 7•7 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?
Assignee | ||
Comment 8•7 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.
Comment 9•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee | ||
Comment 10•7 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.
status-firefox61:
--- → affected
status-firefox-esr60:
--- → fix-optional
Whiteboard: [checkin-needed-beta]
Comment 11•7 years ago
|
||
bugherder uplift |
Whiteboard: [checkin-needed-beta]
Assignee | ||
Comment 12•7 years ago
|
||
To not have our testers experience a hang please also uplift to esr60. Thanks.
Whiteboard: [checkin-needed-esr60]
Comment 13•7 years ago
|
||
bugherder uplift |
Whiteboard: [checkin-needed-esr60]
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•