317476 - The error thrown by JS_ReportError is not catchable

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Blake Kaplan (:mrbkap) (inactive)]

This was brought up by bug 304033. When you call JS_ReportError, the error corresponds to JSMSG_NOT_AN_ERROR, so given the shell function:

static JSBool
ThrowError(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
{
    JS_ReportError(cx, "This is an error");
    return JS_FALSE;
}

And calling it as throwError:
js> try { throwError(); } catch(e) { E = e }
typein:1: This is an error
js> E
typein:2: ReferenceError: E is not defined

I have a patch to fix this.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make it catchable

I don't know if we want the js.c changes. I didn't feel like chasing down uses of JS_ReportError in js.c to test this, but ThrowError is pretty trivial.

Comment 2

[Brendan Eich [:brendan]]

So what message do you get for an uncaught user-defined error report?  The useless one in js.msg, or the one the user passed to JS_ReportError?

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

At least in the JS shell, I get the message passed into JS_ReportError (the expected one), I'm pretty sure I also saw the right behavior when I tested earlier, but I can't test now.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixing the report in the JS console

The old patch showed:
"Error: Error: message"

In the JS console because the report didn't offer a ucmessage so we were using the result of exception.toString() which duplicated the "Error". This patch gives a ucmessage so that clients that just want the wide message can get it.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Please imagine that I JS_free the inflated string.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 204055 [details] [diff] [review]
Fixing the report in the JS console

Why not JSMSG_USER_DEFINED_ERROR?

I'm imagining that inflated string being JS_free'd, but a new patch would be cool.  Too bad we can't suppress the OOM that will nest in any js_ReportErrorVA call that hits a hard limit under js_InflateString.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: with comments addressed

This also nukes a warning about report.ucmessage being a |const jschar *|.

I think the USERDEFINED (no _) came from the htmlparser eHTMLTag_userdefined that I seem to be so used to.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 204336 [details] [diff] [review]
with comments addressed

Rename the "last" local in js_ReportErrorVA to "message" (the name "last" is ancient, and had to do with keeping the last error message owned by the cx on which it was reported), if you can. r=me on that if it's a followup patch.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in, with the additional s/last/message/ renaming.

Comment 10

[Bob Clary [:bc] (inactive)]

Checking in regress-317476.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-317476.js,v  <--  regress-317476.js
initial revision: 1.1
done

Comment 11

[Bob Clary [:bc] (inactive)]

fixed by Bug 336373 on the 1.8.1 branch. 
verified fixed 1.8.1 with windows/macppc/linux 20060707