1107777 - js::ReportError() should set exception on cx if embedder is going to handle it, even if script is not running.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

ReportError() currently gates on JS_IsRunning() before setting the exception on the context. Otherwise it just reports the error. In cases where the embedder sets dontReportUncaught, this leads to it not being unable to handle the exception in cases where script doesn't run, but we use the JSAPI (JS_ParseJSON for example).

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Try to set exception on JSContext if embedder will handle it

If JS is not running, the error reporting infrastructure just reports the
exception to the error reporter. This prevents the embedder from handling it
after setting dontReportUncaught since js_ErrorToException isn't called. This
patch sets the exception even if JS is not running if the embedder will handle
it.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8532327 [details] [diff] [review]
Try to set exception on JSContext if embedder will handle it

this patch is wrong and fails some tests.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Add autoJSAPIOwnsErrorReporting flag to JSContext options. AutoJSAPI sets it

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8537348 [details] [diff] [review]
Add autoJSAPIOwnsErrorReporting flag to JSContext options. AutoJSAPI sets it

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

Look great! thanks for doing this.

::: js/src/jsapi.h
@@ +1640,4 @@
>    private:
>      bool privateIsNSISupports_ : 1;
>      bool dontReportUncaught_ : 1;
> +    bool autoJSAPIOwnsErrorReporting_ : 1;

Add a comment here explaining why we're doing this - dontReportUncaught is kind of random in Gecko at the moment, and we want a clear indicator that the embedding is going to take ownership of error reporting. Also note that this should eventually become the default and both options will go away.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=820cf3b87a5e

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Add autoJSAPIOwnsErrorReporting flag to JSContext options. AutoJSAPI sets it

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe7d8036eac

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7fe7d8036eac

Comment 9

[Johannes Pfrang [:johnp]]

I'm recently (last 1-2 nightlies) getting some crashes with the top third frame showing code that was added with this changeset in bug 1114079.

The crash happens in "js_ErrorToException(cx, message, reportp, callback, userRef)".

I'll set the bug blocking this one, please correct me if I'm wrong.

Comment 10

[Johannes Pfrang [:johnp]]

:nsm

It looks like this changeset caused bug 1114079. Please have a look at the comment above and the linked bug.