remote debugging protocol: handlers should systematically catch and log exceptions

RESOLVED FIXED in Firefox 23

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
Firefox 23
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

It's the established pattern in Firefox for things like the observer service, listener callers, and the like to ignore exceptions thrown by the observer/listener/what-have-you.

(This is kind of like littering, in that's it's a completely avoidable sin, and not even especially satisfying - except that this one can waste hours of someone's time. Not as bad as burglary, but... honestly, people.)

Anyway, another way to look at it is to say that handlers must be infallible. The attached patch makes all the handlers I could find in the remote protocol server infallible, by wrapping them with a function that catches and logs errors. This can then replace the extant one-off catch clauses attempting to address the problem.

Not requesting review yet, as this patch reveals some silent failures. (Whodathunkit!)
Assignee: nobody → jimb
Depends on: 866231
Depends on: 865509
Blocks: 797627
Attachment #741126 - Flags: review?(dcamp)
Comment on attachment 741126 [details] [diff] [review]
debugging protocol: Systematically catch and log exceptions thrown by handler functions that their callers would otherwise ignore.

Review of attachment 741126 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/debugger/dbg-transport.js
@@ +31,5 @@
> + *
> + * (SpiderMonkey does generate good names for anonymous functions, but we
> + * don't have a way to get at them from JavaScript at the moment.)
> + */
> +function MakeInfallible(aHandler, aName) {

Most of our non-constructor functions are camelCase, is this different on purpose?

(also: dbg-transport.js is a weird place for this, but it's the best option for now.  We can fix that soon).
Attachment #741126 - Flags: review?(dcamp) → review+
(In reply to Dave Camp (:dcamp) from comment #1)
> Most of our non-constructor functions are camelCase, is this different on
> purpose?

No; I've changed it. Thanks for pointing this out.

> (also: dbg-transport.js is a weird place for this, but it's the best option
> for now.  We can fix that soon).

Yeah; it's the only thing shared between dbg-client.jsm and the server-side stuff. It would be nice to have a devtools utilities file... although those always turn into junk drawers, or grow more structure than they ought.
Group: mozilla-corporation-confidential
Group: mozilla-corporation-confidential
https://hg.mozilla.org/integration/mozilla-inbound/rev/77ca8f2074c9
Status: NEW → ASSIGNED
Flags: in-testsuite-
Target Milestone: --- → Firefox 23
https://hg.mozilla.org/mozilla-central/rev/77ca8f2074c9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.