Marionette hangs if errors are thrown and not handled in content listeners using the old dispatching technique

RESOLVED FIXED in Firefox 54

Status

defect
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Version 3
mozilla55
Points:
---

Firefox Tracking Flags

(firefox-esr52 fix-optional, firefox54 fixed, firefox55 fixed)

Details

Attachments

(1 attachment)

While working on bug 1335778 I pushed a try build for my patch. As seen it has a lot of test failures:

https://treeherder.mozilla.org/#/jobs?repo=try&author=hskupin@mozilla.com

Since then I was investigating the problem, and it turns out it is a hang for those listener commands which use the old dispatching technique. This hang specifically happens when any kind of exception is thrown right in the function body eg. listener.js:get().

It looks like sending the reply works fine. At least I do not get a failure. Maybe receiving it on the chrome side has a problem. And as result we never resolve the promise.

I haven't figured out yet where the reply is handled on the chrome side. I tried in proxy.js but was not that successful.

Andreas, do you have an advice where I can have a look at?
Flags: needinfo?(ato)
It has taken me a while to understand the old dispatching technique but now I got it. I'm sure that I know now what has to be done here.
Flags: needinfo?(ato)
Comment on attachment 8857855 [details]
Bug 1356000 - Ensure unwrapped content listeners catch errors.

https://reviewboard.mozilla.org/r/129890/#review132508

::: commit-message-f40e2:1
(Diff revision 1)
> +Bug 1356000 - Content listeners with old dispatching technique have to return errors.

This is a bit long, maybe this will do?

> Ensure unwrapped content listeners catch errors

::: commit-message-f40e2:3
(Diff revision 1)
> +Currently content listeners which are using the old dispatching technique
> +can cause a hang of Marionette if an exception is thrown. To ensure that
> +errors are getting returned to the chrome process, all the code has to be
> +placed into a try/catch block.

I suggest this rephrasing:

> Content listeners that are using the old IPC dispatching technique can
> cause Marionette to hang when errors are thrown but not handled.  To
> ensure errors are returned to the chrome process, all the code has to
> be placed in try…catch blocks.
Attachment #8857855 - Flags: review?(ato) → review+
Summary: Marionette hangs if exception gets thrown in functions which use the old dispatching technique → Marionette hangs if errors are thrown and not handled in content listeners using the old dispatching technique
Assignee: nobody → hskupin
No longer blocks: 1335778
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb778a3d681e
Ensure unwrapped content listeners catch errors. r=ato
https://hg.mozilla.org/mozilla-central/rev/bb778a3d681e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Test-only fix which prevents a hang of Marionette, and which we would like to have on beta and esr52. Please uplift. Thanks.
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]
https://hg.mozilla.org/releases/mozilla-beta/rev/2c92847dca76
Whiteboard: [checkin-needed-beta][checkin-needed-esr52] → [checkin-needed-esr52]
Needs rebasing for ESR52.
Flags: needinfo?(hskupin)
Whiteboard: [checkin-needed-esr52]
Lets not worry about esr52 then. If it turns out to become an issue, we can do the update of the patch and its uplift.
Flags: needinfo?(hskupin)
You need to log in before you can comment on or make changes to this bug.