Closed
Bug 1356000
Opened 7 years ago
Closed 7 years ago
Marionette hangs if errors are thrown and not handled in content listeners using the old dispatching technique
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox-esr52 fix-optional, firefox54 fixed, firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | fix-optional |
firefox54 | --- | fixed |
firefox55 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
Details
Attachments
(1 file)
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)
Assignee | ||
Comment 1•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
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+
Updated•7 years ago
|
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
Updated•7 years ago
|
Assignee: nobody → hskupin
Comment hidden (mozreview-request) |
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bb778a3d681e Ensure unwrapped content listeners catch errors. r=ato
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bb778a3d681e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 7•7 years ago
|
||
Test-only fix which prevents a hang of Marionette, and which we would like to have on beta and esr52. Please uplift. Thanks.
status-firefox54:
--- → affected
status-firefox-esr52:
--- → affected
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]
Comment 8•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2c92847dca76
Whiteboard: [checkin-needed-beta][checkin-needed-esr52] → [checkin-needed-esr52]
Comment 9•7 years ago
|
||
Needs rebasing for ESR52.
Flags: needinfo?(hskupin)
Whiteboard: [checkin-needed-esr52]
Assignee | ||
Comment 10•7 years ago
|
||
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)
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•