981187 - Make the entire JS engine dontReportUncaught and handle exceptions in the embedding

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

See bug 979926 comment 6. Basically, we should add a method on AutoEntryScript like .setPropagateUncaught(bool). We should probably make the default |false| here, so that we can assert against setting |true| when there's nothing else on the stack to propagate the exception to (i.e. when the previous entry is null/System).

When a AutoEntryScript comes off the stack, we check for a pending exception. If it exists, we either invoke an error reporter directly, or stick the exception on the previous JSContext. Then, we kill js_ReportUncaughtException. \o/

I don't think we can do this until we actually use the Script Settings Stack everywhere (instead of just pushing raw JSContexts), so this is blocked on bug 951991.

We'll need to do something different on Workers, but it should be relatively straightforward.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is actually a bit of a pain to do with the current JSAPI.  The current APIs explicitly have "report" functions whose semantics are actually "report unless we're running script; then set a pending exception".  I would love it if we'd start by separating "set pending exception" APIs from "report" APIs.

Comment 2

[Luke Wagner [:luke]]

It seems like part of this bug or a bug blocking this would be adding these new and improved SetPendingException JSAPIs.

I think you'd also need an enhanced version of JS_GetPendingException: when we create an error object, we also create a hidden JSErrorReport and attach this to the exception in a reserved slot.  The JSErrorReport is only used if the exception is uncaught and it contains a bit of extra info that you can't get from the exception object itself.  Thus, if you want to remove JS_ReportPendingException/js_ReportUncaughtException, you'd need this enhanced GetPendingException to provide the associated JSErrorReport (if it exists).

Comment 3

[Terrence Cole [:terrence]]

Attachment: implement_sane_error_api-v0.diff

This patch introduces JS::ThrowException, JS::ReportWarning, and JS::ReportStrictWarning. Throw does SendPendingException and report calls the error reporter, modulo WERROR and strict error reporting options.

What is here is a good start, but it is not yet sufficient for what gecko (or the GC) need from it.

Most importantly, the distinction between JSErrorReport, ErrorObject, and ExceptionObject is pretty much totally bogus. I need to spend some "quality" time with our various reporter callbacks to find the right set of properties to expose and how. In particular, both reports and exceptions end up going through the report callback, so it seems silly to lazily create either the exception or the report, much less both. We generally end up creating at least 3 simultaneous copies of the error report string, sometimes more.

Off thread parsing: the current patch is capable of replacing PJS exceptions -- a nice simplification -- but it totally ignores off-thread-parsing errors. These will need attention before I get too much further.

Error args: I don't even. The only real user is gecko's own js.msg, which appears to have copied our own terrible ideas wholesale. This is a pretty high priority for me to kill off. The test that uses them is just insane and needs to die ingloriously.

Uncatchable exceptions: We currently represent these by JSEXN_NONE in the report and no pending exception. This means the API user has to have two error paths, when one should be sufficient.

There are probably other things I've yet to uncover as well. I expect there will be some semantic changes as we convert stuff to use the new algorithm, but I'm not terribly worried about this: our current API is too confusing to use correctly, and thus simply isn't in most places. In the hour or so I've spent using the new API to replace the old, the vast majority of the sites I've replaced have had something subtly wrong with them. We'll see how things pan out as we replace more.

Comment 4

[Terrence Cole [:terrence]]

Attachment: replace_some_reporting-v0.diff

Uses the new api to replace mostly warning sites.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8484611 [details] [diff] [review]
implement_sane_error_api-v0.diff

At Bobby's request, I'm moving these to a JS-api specific bug.

Comment 7

[Jan de Mooij [:jandem]]

This is done now (bug 1277278 removed dontReportUncaught).