Closed Bug 1595046 Opened 1 year ago Closed 6 months ago

Make uncaught exceptions values inspectable in the WebConsole

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: julienw, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

(Whiteboard: [dt-q])

Attachments

(2 files, 1 obsolete file)

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

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

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

Priority: -- → P2
See Also: → 1595152

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

Component: Console → JavaScript Engine
Product: DevTools → Core
Flags: needinfo?(jdemooij)

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

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?

Flags: needinfo?(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.

Flags: needinfo?(bzbarsky)
Duplicate of this bug: 1595452

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.

Whiteboard: [dt-q]

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?

Flags: needinfo?(loganfsmyth)

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.

Flags: needinfo?(jdemooij)

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.

Flags: needinfo?(loganfsmyth)

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.

Flags: needinfo?(sdetar)
Depends on: 1549641

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.

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.

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?

Flags: needinfo?(nchevobbe)

I think Bug 1595152 would help for that

Flags: needinfo?(nchevobbe)

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.

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.

Flags: needinfo?(sdetar)

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.

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

Flags: needinfo?(nchevobbe)
Attachment #9129258 - Attachment description: Bug 1595046 - Hack → Bug 1595046 - WIP: Make it possible to inspect every exception value in the web console.
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED

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.

Blocks: 1631114

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 ?

(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 :)

Flags: needinfo?(nchevobbe)
Summary: Different behavior between `throw` and `await Promise.reject` about how errors are displayed → Make uncaught exceptions values inspectable in the WebConsole
No longer blocks: 1631114
Depends on: 1631114

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

Attachment #9129258 - Attachment description: Bug 1595046 - WIP: Make it possible to inspect every exception value in the web console. → Bug 1595046 - Make it possible to inspect every exception value in the web console.

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

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.

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

Attachment #9144474 - Attachment is obsolete: true

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.

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 :)

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

Attachment #9141188 - Attachment description: Bug 1595046 - [WIP] Display thrown object as inspectable elements. r=evilpie. → Bug 1595046 - Display thrown object as inspectable elements. r=evilpie, Honza!.
Attachment #9141188 - Attachment description: Bug 1595046 - Display thrown object as inspectable elements. r=evilpie, Honza!. → Bug 1595046 - Display thrown object as inspectable elements. r=evilpie,Honza!.
Keywords: leave-open
Blocks: 1636590
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/58bcc2778e01
Make it possible to inspect every exception value in the web console. r=jonco,baku,mccr8
Regressions: 1636624
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7df79387af7
Display thrown object as inspectable elements. r=Honza.
Keywords: leave-open
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84c55ea8397f
Display thrown object as inspectable elements. r=Honza,perftest-reviewers,davehunt.

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

Flags: needinfo?(nchevobbe)
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Regressions: 1636995

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.

Flags: needinfo?(evilpies)

No. I think we can fix this.

Flags: needinfo?(evilpies)
You need to log in before you can comment on or make changes to this bug.