1066244 - Kill JSErrorReport and replace with exception object queries

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

The plan is to make embeddings catch exceptions themselves so that they can choose to report the exception, propagate to the caller, or swallow the exception -- as opposed to letting us guess incorrectly what needs to be done. This is bug 981187. This bug is the JS half of that work.

Right now, the embedding can get the exception object and/or call JS_ReportUncaughtException. The exception object is of questionable utility, since it is potentially unstructured and relies on the engine to fill in the details. On the other hand, calling JS_ReportUncaughtException may or may not do the right thing.

As a first step, we want to expose a new, temporary api, JS::ExceptionToErrorReport, and remove JS_ReportUncaughtException (and ideally AutoLastFrameCheck too). This will let the embedding call the error/warning reporter itself, so that it doesn't have to rely on SM to do the right thing.

As a second step, we want to expose the duck typing bits directly so that the embedding can go straight from an exception object to the properties that xpc::ErrorReport needs. This will be API's like JS::FilenameFromException and such; see xpc::ErrorReport for the full set.

At this point, JSErrorReport will only be used for warning reports, so we can simplify the structure significantly. We'll want to introduce a new, simpler JS::WarningReport structure with fixed memory management, fix the API names to reflect the new usage, and possibly other improvements.

Comment 1

[Bobby Holley (:bholley)]

Beautifully written! Thanks for doing that.

Comment 2

[Terrence Cole [:terrence]]

(In reply to Bobby Holley (:bholley) from comment #1)
> Beautifully written! Thanks for doing that.

Sorry it's taken me so long to figure out what's what with this series of bugs! As Jason said on IRC yesterday, "when working on SpiderMonkey you usually don't know what patch is first in a series until you're 3 patches in."

Comment 3

[Terrence Cole [:terrence]]

Attachment: expose_exception_to_error-v0.diff

This should be step 1.

Bobby, does the test case here do basically what you expect the new usage to look like?

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8488269 [details] [diff] [review]
expose_exception_to_error-v0.diff

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

::: js/src/jsexn.cpp
@@ +625,5 @@
> +    MOZ_ASSERT(!cx->isExceptionPending());
> +    ErrorReport err(cx);
> +    if (!err.init(cx, exn))
> +        return false;
> +    *reportp = *err.report();

Doesn't this leave the pointers dangling when |err| goes off  the stack?

More to the point though, I just noticed that ErrorReport (a New Thing) is externally-exposed via jsfriendapi. So I _think_ we should have all the tools we need, and the caller can just do this munging directly with JSAPI.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Bobby Holley (:bholley) from comment #4)
> Comment on attachment 8488269 [details] [diff] [review]
> expose_exception_to_error-v0.diff
> 
> Review of attachment 8488269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsexn.cpp
> @@ +625,5 @@
> > +    MOZ_ASSERT(!cx->isExceptionPending());
> > +    ErrorReport err(cx);
> > +    if (!err.init(cx, exn))
> > +        return false;
> > +    *reportp = *err.report();
> 
> Doesn't this leave the pointers dangling when |err| goes off  the stack?

D'oh! I totally forgot that I haven't made these UniquePtr yet, despite the definition being /right there/.

> More to the point though, I just noticed that ErrorReport (a New Thing) is
> externally-exposed via jsfriendapi. So I _think_ we should have all the
> tools we need, and the caller can just do this munging directly with JSAPI.

Good catch! I really did not expect that to be in friend api.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, I added ErrorReport as a friend API precisely so I could have Gecko do the same exception munging the engine was already doing.  ;)