1795474 - Review DevTools helpers makeInfallible and reportException

Status: ASSIGNED

(DevTools :: General, task, P3)

Description

[Julian Descottes [:jdescottes]]

It's not clear if those helpers are really useful, and since they are used by the Network Observer codebase, we might start leaking them to the remote/bidi codebase if we want to start sharing those modules.

We should review their usefulness before using the network observer modules in bidi.

Note that they have been duplicated to an ES Module called DevToolsInfaillibleUtils.sys.mjs because they also need to be used from workers and workers cannot load modules for now.

So if we could stop using them in the network observer codebase, we would also avoid this duplication.

Comment 1

[Julian Descottes [:jdescottes]]

See https://phabricator.services.mozilla.com/D156968#5226779 for more details as well.

Comment 2

[Julian Descottes [:jdescottes]]

makeInfaillible is basically wrapping a method in a try catch and allows the consumer to provide a helpful name/message that should be logged when the method fails. It's very rare that we actually use it for swallowing errors, because it's mostly used as an event listener callback or in combination with Services.tm.dispatchToMainThread (or any other helper allowing to wait for a tick)

There are exceptions, eg JSPropertyProvider, but it will be easy to migrate the few call sites which actually need the "infaillible" behavior to use a regular try/catch.

So the main question is whether the added wrapper really makes the reporting better. I took randomly used a call site in local-transport.js and added a plain JS error in the middle of the method.

With makeInfaillible/executeSoon:

Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: ReferenceError: a is not defined
Stack: send/<@resource://devtools/shared/transport/local-transport.js:67:11
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:103:22
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:103:22
Line: 67, column: 11
console.error: (new ReferenceError("a is not defined", "resource://devtools/shared/transport/local-transport.js", 67))

Without makeInfaillible/executeSoon:

JavaScript error: resource://devtools/shared/transport/local-transport.js, line 67: ReferenceError: a is not defined

The main difference here is that we are missing the stacktrace in stdout, you will only get a single report line for the error in the function / module which happened to fail. Here I threw in local-transport directly, but if the error was nested in a random module, you might very easily dismiss the error. You will have the stacktrace in the browser toolbox / console, but it's still going to be harder to find out that local-transport is the one which failed.

I think there is value in having our key framework handlers/callbacks report meaningful errors and easy to spot errors. At least it's worth discussing.

The other question is whether it is safe for us to throw in platform handlers or if that could lead to issues. Not sure if this is the case or not, hard to tell if the main motivation for this helper was to shield platform handlers or to make framework errors easier to spot.

Comment 3

[Julian Descottes [:jdescottes]]

reportException does two things: it attempts to stringify the exception/error provided to the method (including the stack and location) and will then use dump to print it.

And if console.error is available, it will also call console.error(exception) (note that for this second part we directly use the exception which was provided as an argument of the method, not the stringified version).

The dump call will consistently print the information to stdout, where as the console.error call will only provide detailed information when using the Browser Toolbox / Browser Console.

So there are two questions around this one:

• do we still have environments where console (or console.error) is unavailable?
• do we care about having stacks & locations available directly in stdout, or is it good enough to use console.error?