1595046 - Make uncaught exceptions values inspectable in the WebConsole

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Julien Wajsberg [:julienw]]

This is OK:

throw new Error("message")
// => Error: message

await Promise.reject(new Error("message"))
// => Error: message

This isn't OK:

throw {name:"Error",message:"message"}
// => [object Object]

await Promise.reject({name:"Error",message:"message"})
// => uncaught exception: Object


var error = new Error("message")
error.name = "foo"

throw error
// => foo: message

await Promise.reject(error)
// => Error: message

Comment 1

[Julien Wajsberg [:julienw]]

It's especially unfortunate that error.name isn't displayed in the last case. This is what annoys me the most.

Comment 2

[:Harald Kirschner :digitarald]

This would massively help DevTools as well, as we are having trouble following up cryptic error messages.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

I think the one for error.name might be the same as Bug 1549641.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Note that you can still observe issues without await:

Promise.reject({name:"Error",message:"message"})

produces uncaught exception: Object

And

 var error = new Error("message")
error.name = "foo"
Promise.reject(error)

produces Error: message

Comment 5

[Jan de Mooij [:jandem]]

There are multiple issues here, but one thing I notice compared to Chrome is that they show the object in the console and let you 'interact' with it, whereas we just show a string representation.

In Chrome, A and B appear to be equivalent in the console:

var err = {message: "Something", name: "Error", foo: "bar"};
console.error("Uncaught", err); // A
throw err; // B

In Firefox, A is great (you can even inspect the object!) but B is pretty bad (just a string).

How feasible is it to make B behave more like A internally? Can we somehow avoid nsScriptErrorWithStack and act more like console.error was called?

Comment 6

[Boris Zbarsky [:bzbarsky]]

How feasible is it to make B behave more like A internally?

That's a good question... Right now what happens is we init a js::ErrorReport from the error, and then only examine the resulting JSErrorReport, which does not include a reference to the underlying error, so at that point we have already lost in terms of achieving this goal.

We could rejigger things to keep propagating the actual exception object along, storing it in the xpc::ErrorReport, then storing it in the nsScriptErrorWithStack and then the devtools frontend can presumably do whatever. I was worrying about it leaking, but I guess it wouldn't really leak any worse than the stack thing does already...

I don't know what the right thing to do for workers would be, and don't recall offhand how console.error in workers gets handled right now.

Sharing the same underlying implementation as console.error (i.e. the console storage bits) seems a bit difficult but maybe it's possible... The main issue is that there are things observing exceptions that expect to get them (via the console service) but iirc console.error doesn't do anything with that.

Comment 8

[Jan de Mooij [:jandem]]

Heads-up: I'll see if I can prototype passing the exception object to the console somehow. I'm not very familiar with this code and I'm working on a number of other things so it might take a while.

Comment 9

[:Harald Kirschner :digitarald]

Another occurrence in the wild: https://twitter.com/wesbos/status/1202334015841280003

Comment 10

[:Harald Kirschner :digitarald]

Logan, would be great to have your input here to get an idea of root cause & scope. Is this related to the async stack work?

Comment 11

[Jan de Mooij [:jandem]]

Sorry I don't have time to look into this anytime soon. I still think the best way to fix these bugs is to pass the exception object somehow so we can show it and its properties.

Comment 12

[Logan Smyth [:loganfsmyth]]

I agree with Jandem that it seems like the thing to do here would be to figure out how to pass the error object through so that the console can access it and use the actual object to render things in the console.

As mentioned, we could implement https://bugzilla.mozilla.org/show_bug.cgi?id=1549641 as a smaller fix too.

This work is not related to async stacks.

Comment 13

[:Harald Kirschner :digitarald]

Steven, it seems like we could stem the related bug 1549641 on our side at some point, but for this bug, it would be great to have help from the JS team.

This comes up in our user feedback survey but also affects Firefox frontend engineers that work in async code.

Comment 14

[Tom S [:evilpie]]

I first though we could maybe improve the situation for throw {name: "foo", message: "bar"} by changing the SpiderMonkey code in ErrorReport::init, and while can certainly improve this, it also seems to go against the current spirit of the code. In IsDuckTypedErrorObject we check for message, file{n,N}ame and lineNumber properties to detect duck-typed error objects. So just name + message is currently not enough. To support the Promise.reject case we would additionally need to change the code to work without side-effects.

I think passing the thrown value, by extending nsScriptErrorWithStack should be feasible. I wonder if we also want to support that with nsScriptError, but the former at least already has all the required rooting for GC pointers so that seems easier to get correct. I would also prefer to keep around values (i.e. throw 123) instead of just (error) objects. If we work on this we might as well do it right.

Comment 15

[Tom S [:evilpie]]

Attachment: Bug 1595046 - Make it possible to inspect every exception value in the web console.

Comment 16

[Tom S [:evilpie]]

I hacked something together that shows that this is definitely feasible. I assume someone else can fix up the devtools part to correctly extend the PageError message. nsScriptErrorWithStack has this stackGlobal and all kinds of scary comments about compartments etc., which will probably also apply to exception objects. So actually getting this in a landable state is a lot more work.

Comment 17

[:Harald Kirschner :digitarald]

Nicolas, do you have a good sense on what's needed on the UI side to show the right data? Should this be spun out in its own bug?

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

I think Bug 1595152 would help for that

Comment 19

[:Harald Kirschner :digitarald]

I think Bug 1595152 would help for that

Not sure. Afaik Bug 1595152 is about inspectability of custom properties on error objects; while this work is meant to show the errors coming through promises.

Comment 20

[Steven DeTar [:sdetar]]

Harald and I discussed this, we should get the right folks together to understand the changes required so that we can get this on the SM teams radar so we can prioritize it for near future work.

Comment 21

[Tom S [:evilpie]]

I am willing to work on this. However I do need someone from the devtools team to work on actually exposing this to users. I think my patch is actually not too far away from something we could land.

Comment 22

[:Harald Kirschner :digitarald]

ni? Nicolas as liason as he was already involved in the chats on matrix.

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1595046 - Display thrown object as inspectable elements. r=evilpie,Honza!.

Depends on D64437

Comment 24

[Tom S [:evilpie]]

I just updated my C++ patch. We now use Maybe<JS::Value> to differentiate between "we don't have an exception value available" and undefined. Before these two cases were both just undefined. I also added hasException to nsIScriptError for this.
Furthermore we now support saving the exception value for main-thread promise rejections! I actually think the patch is already in a pretty good state. It's probably time soon to get some feedback on the basic approach of this patch.

Comment 25

[Tom S [:evilpie]]

Because AsyncErrorReporter can now produce an nsScriptErrorWithStack with an exception but /without/ a stack, the name is now misleading. I can't immediately think of a better name however, maybe something like nsScriptErrorWithJS ?

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tom Schuster [:evilpie] from comment #25)

Because AsyncErrorReporter can now produce an nsScriptErrorWithStack with an exception but /without/ a stack, the name is now misleading. I can't immediately think of a better name however, maybe something like nsScriptErrorWithJS ?

that sounds reasonable to me, but I guess this can be discuss by a peer at review time :)

Comment 27

[Tom S [:evilpie]]

Did some cleanup after bug 1631114 landed, hope we can move this forward soon.

Comment 28

[Tom S [:evilpie]]

I think my patch might be causing some additional leaks presumably because the nsScriptErrorWithStack is keeping some global alive.

Comment 29

[Tom S [:evilpie]]

At least browser/components/contextualidentity/test/browser/browser_serviceworkers.js seems to easily reproduce locally for me. I however don't have any experience with leaks in the browser.

Comment 30

[Tom S [:evilpie]]

On a hunch I removed the SetException call in Promise::ReportRejectedPromise, because that is probably the only place were we are now retaining an additional reference to some JSObjects. (Currently no support for stacks...) This fixes at least the leak above locally.
mccr8 suggest that we need to cleanup with runnable when the inner window is destroyed similar to: https://searchfox.org/mozilla-central/rev/2bfe3415fb3a2fba9b1c694bc0b376365e086927/xpcom/base/nsConsoleService.cpp#522-529

Comment 31

[Tom S [:evilpie]]

Attachment: Bug 1595046 - Try to fix leak with AsyncErrorReporter

Comment 32

[Tom S [:evilpie]]

It seems like we can get away with just checking window->IsDying() before setting an exception on the AsyncErrorReport. This try run looks notably better, at least I think so, the dozens or so of what look like intermittent failures make this a daunting task. However we still get one reproducible leak in bc13 / browser/base/content/test/siteIdentity/browser_secure_transport_insecure_scheme.js. This test doesn't seem to use AsyncErrorReport at all, so we are back at trying to guess what is going on.

Comment 33

[Tom S [:evilpie]]

nsXPCComponents_Utils::ReportError would create nsScriptErrorWithStack instances referencing some exception value, but without any associated window. This would cause that exception to leak. We can just not do that :)

Comment 34

[Tom S [:evilpie]]

And yet another case: ScriptErrorEvent::Run can also run on a dying global.

Comment 35

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1329cda1a30f760617472813a214790a0b03843a

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58bcc2778e01
Make it possible to inspect every exception value in the web console. r=jonco,baku,mccr8

Comment 37

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/58bcc2778e01

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7df79387af7
Display thrown object as inspectable elements. r=Honza.

Comment 39

[Dorel Luca [:dluca]]

Backed out changeset d7df79387af7 (bug 1595046) for Talos tests failures in webconsole/complicated.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=301558774&repo=autoland&lineNumber=1059

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=d7df79387af7b5adec14a87b03b79b1f7efcf3cd

Backout:
https://hg.mozilla.org/integration/autoland/rev/24419100cf9d73120c38ae8a72c51f050842d945

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84c55ea8397f
Display thrown object as inspectable elements. r=Honza,perftest-reviewers,davehunt.

Comment 41

[Nicolas Chevobbe [:nchevobbe]]

there was a failure in a damp test, I fixed it

Comment 42

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/84c55ea8397f

Comment 43

[Andrew McCreight [:mccr8]]

Can you please back this out? It isn't good to have such a high-frequency intermittent failure in the tree for so long. Thanks.

Comment 44

[Tom S [:evilpie]]

No. I think we can fix this.