215173 - JS_ReportErrorNumber doesn't raise the user-specified exception

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Braden]

JS_ReportErrorNumber doesn't raise the user-specified exception (from the 
user's js.msg equivalent). Instead, it raises whatever exception corresponds 
to the error number in SpiderMonkey's js.msg.

Comment 1

[Phil Schwartau]

cc'ing Brendan, Mike.

Braden: can you give sample code, showing how you are calling
JS_ReportErrorNumber? Thanks -

Comment 2

[Braden]

Probably. But I haven't actually written such code yet; I've just been reading 
the SpiderMonkey sources.

For JS_HAS_ERROR_EXCEPTIONS, a call to JS_ReportErrorNumber results in 
js_ErrorToException being called. js_ErrorToException *always* pulls the 
JSExnType from an array created from SpiderMonkey's js.msg.

Comment 3

[Steven Cole]

In other words, the public API JS_ReportErrorNumber can only report JS-Engine
internal errors.  Embeddings of the JS Engine must use some other technique to
report their own errors.

So the RFE here is something like: Provide an API to register new error messages
to the JS Engine (perhaps one large group of messages), so that the numerical
range of those messages does not conflict with the JS-Engine errors, and so that
API users may then call JS_ReportErrorNumber (or perhaps some other new
function) with some appropriate value to cause the engine to emit the newly
registered messages.

It might also be good to expose the Error prototype via the API so that
embeddings can throw real Error objects, or objects derived from the Error object.

(Our workaround was to call 'Error' as a function, since the ECMA specification
says that such a call does the same thing as running the constructor.)

--scole

Comment 4

[Braden]

> In other words, the public API JS_ReportErrorNumber can only report
> JS-Engine internal errors.  Embeddings of the JS Engine must use some 
> other technique to report their own errors.

Right. That doesn't appear to have been the intent; but it is a result of the 
way things are implemented. (And it doesn't look like this can be changed 
without changing the API.) The usage pattern employed in js.c cannot work 
reliably.

Regardless of whatever is added, the existing JS_ReportErrorNumber[UC] and 
JS_ReportErrorFlagsAndNumber[UC] ought to be deprecated, as it doesn't seem 
like they can work correctly.

The most straightforward way to fix this is to extend the existing pattern by 
adding another callback to get the JSExnType. (The callback would fill the 
role now occupied by the internal function js_ErrorToException.) As Brendan 
points out on n.p.m.jseng, the names for the new API functions will likely be 
long and ugly because the good names have already been taken by the broken API 
functions.

Comment 5

[Brendan Eich [:brendan]]

Must stop the madness, now.

/be

Comment 6

[Brendan Eich [:brendan]]

JS_ReportError doesn't result in error-as-exception at all, which is unfortunate.

/be

Comment 7

[Michael Daumling]

Recommended changes:

1) Export JSExnType from jsexn.c to jspubtd.h
2a) Redefine JSErrorFormatString as follows:
       uintN argCount; // upper 16 bits can contain the exception type
2b) Alternatively, add a JSExnType field to JSErrorFormatString. Would be a good
idea to cast argCount and exnType to JSUInt8 to save space:
typedef struct JSErrorFormatString {
    const char *format;
    JSUint8 argCount;
    JSUint8 exnType; 
} JSErrorFormatString;
3) Change JSLocaleCallbacks to:
struct JSLocaleCallbacks {
    JSLocaleToUpperCase     localeToUpperCase;
    JSLocaleToLowerCase     localeToLowerCase;
    JSLocaleCompare         localeCompare;
    JSLocaleToUnicode       localeToUnicode;
    JSErrorCallback	    localeErrorCallback; // new
};
4) Allow for the JSErrorCallback in all error reporting mechanisms to be NULL.
This causes the localeErrorCallback to be used, and, if not present (may be NULL
to preserve backwards compatibility), the built-in callback.

Effects:

1) Preserves API compatibility, while alllowing to pass in NULL to reporters for
the error callback.
2) Most people that use JSLocaleCallbacks define this as static struct, which
would leave the errorCallback member as NULL. Struct compatibility is preserved.
3) Full localizability.

Let me know what you think, assign the bug to me, and I'll submit patches.

Comment 8

[Brendan Eich [:brendan]]

Michael has kindly offered to take this bug.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Recommended changes:
> 
> 1) Export JSExnType from jsexn.c to jspubtd.h

This is ok, it's an additive API change within our JS prefix "poor-man's C type
namespace".

> 2a) Redefine JSErrorFormatString as follows:
>        uintN argCount; // upper 16 bits can contain the exception type

Hacky, presumably API backward-compatible, since existing implementations of the
JSErrorCallback signature will return something with 0 in the high 16 bits -- or
on 64-bit architectures, 0 in the high 48 bits.  Needs documenting and perhaps
some macro helpers to crack and combine, if we go this way.

> 2b) Alternatively, add a JSExnType field to JSErrorFormatString. Would be a good
> idea to cast argCount and exnType to JSUInt8 to save space:
> typedef struct JSErrorFormatString {
>     const char *format;
>     JSUint8 argCount;
>     JSUint8 exnType; 
> } JSErrorFormatString;

Not API backward-binary-compatible.  Is this bad?  We have broken binary
backward incompatibility by extending structs beyond their reserved members when
space ran out (JSClass at one point long ago; JSObjectOps).  I never heard
complaints, and it seems people recompile with each update to a new SpiderMonkey.

So I think we could do this, and it is less hacky than (2a).  I'd like to hear
from anyone dealing with binary compatibility first, in this bug and quickly, if
there is a real concern.

> 3) Change JSLocaleCallbacks to:
> struct JSLocaleCallbacks {
>     JSLocaleToUpperCase     localeToUpperCase;
>     JSLocaleToLowerCase     localeToLowerCase;
>     JSLocaleCompare         localeCompare;
>     JSLocaleToUnicode       localeToUnicode;
>     JSErrorCallback	    localeErrorCallback; // new
> };

Additive, but old implementations recompiled with the new header might just
work, without supplying an initializer (some compilers would warn).  Ah, you
have this covered below:

> 4) Allow for the JSErrorCallback in all error reporting mechanisms to be NULL.
> This causes the localeErrorCallback to be used, and, if not present (may be NULL
> to preserve backwards compatibility), the built-in callback.

Sounds great, assuming (2b) is the way to go.

/be

Comment 10

[Michael Daumling]

Re 2b): I could just add another intN element to JSErrorFormatString, doing a
pure additive change, but that would increase code size by 800 bytes (assuming a
table of 200 messages, with two four-byte values vs two 1-byte values and
assumed padding to a multiple of 4 bytes). On embedded systems, the gain is even
more, assuming a possible packing at 1 or 2 bte boundaries. Please vote: save
some space, or add another intN member? IMHO, this structure has probably not
been used that much before.

Comment 11

[Brendan Eich [:brendan]]

May as well use two uint16 members, to allow for insane future growth and to
consume typical RISC-y/superscalar-modern-CPU padding.  I think we can take the
compatibility hit, on second thought, since this bug stands in the way of anyone
making effective use of an embedding-specific message catalog.

/be

Comment 12

[Michael Daumling]

The JSErrorFormatString needs to be more complex - the current version does not
support Unicode. It does not make sense to create a Unicode-only version,
becasue js.msg would not work anymore.

typedef struct JSErrorFormatString {
   union {
      const char   *format;    /* message string */
      const jschar *ucformat;  /* message string, Unicode */
   } msg;
   JSUint8 formatTag;          /* 0 - const char*, 1 - const jschar* */
   JSUInt8 argCount;           /* number of replacable parts in message */
   JSInt16 exnType;            /* exception type (one of JSEXN_* */
} JSErrorFormatString;

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> The JSErrorFormatString needs to be more complex - the current version does not
> support Unicode. It does not make sense to create a Unicode-only version,
> becasue js.msg would not work anymore.
> 
> typedef struct JSErrorFormatString {
>    union {
>       const char   *format;    /* message string */
>       const jschar *ucformat;  /* message string, Unicode */
>    } msg;

C won't let you initialize a union, so we'd have to initialize at runtime.

>    JSUint8 formatTag;          /* 0 - const char*, 1 - const jschar* */

Please use uint8, etc. as the rest of the code does.

The JS* aliases come from an importing (really forking) process done once to
bring NSPR header files into the JS engine, so it could stand alone, even of
NSPR.  I was helping found mozilla.org then and didn't object loudly enough, but
it does make for unwanted choice and namespace "brainprint".

Let's just pretend that didn't happen, and keep using the simple all-lowercase
typedef names. ;-)

>    JSUInt8 argCount;           /* number of replacable parts in message */
>    JSInt16 exnType;            /* exception type (one of JSEXN_* */
> } JSErrorFormatString;

I have a proposal: keep const char *format but let it be UTF-8 by definition,
given your impending compile-time option to support UTF-8 (bug 232182).

/be

Comment 14

[Michael Daumling]

Attachment: Bug fix as discussed

This patch fixes the bug as discussed. The JSErrorFormatString struct has now three members, the char* message, the argc (now int16 instead of uintN), and the exception type (new, int16). The errorCallback argument to all error reporters may now be NULL, which causes a new error callback, defined in JSLocaleCallbacks as a new member. All internal error reporters attempt to call this function before calling js_GetErrorMessage (to avoid too many changes to the code base, the same mechanism is invoked if js_GetErrorMessage is supplied as callback, as all internal errors do). Users can register this callback to define their truly own error codes, exception type, and localized error messages (message must be UTF-8 for extended charsets, and source must have been compiled with JS_STRINGS_ARE_UTF8).

No testcase supplied - a testcase would require a new, hardcoded error code to test against, which is not practical in the browser environment for testing.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 202066 [details] [diff] [review]
Bug fix as discussed

>Index: jscntxt.c
>@@ -816,120 +816,122 @@ js_ExpandErrorArguments(JSContext *cx, J
>+    /* make it easier on code changes: if callback == js_GetErrorMessage, assume NULL */

This comment doesn't make much sense to me (and doesn't start with a capital or end with a period).

>+    if (!callback || callback == js_GetErrorMessage)
>+        efs = js_GetLocalizedErrorMessage (cx, userRef, NULL, errorNumber);
>+    else
>         efs = callback(userRef, NULL, errorNumber);
>-        if (efs) {
>-            size_t totalArgsLength = 0;

This stuff is all just being outdented, right?

>+                * Gather the arguments into an array, and accumulate
>+                * their sizes. We allocate 1 more than necessary and
>+                * null it out to act as the caboose when we free the
>+                * pointers later.
>+                */

Nit: This comment is misindented.

>+            /*
>+            * Zero arguments: the format string (if it exists) is the
>+            * entire message.
>+            */

Super-nit: The *s want to line up.

Comment 16

[Michael Daumling]

Brendan, this patch is getting very old. Can you, please, super-review?

Comment 17

[Michael Daumling]

Attachment: Updated patch with nits and super-nits.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 204074 [details] [diff] [review]
Updated patch with nits and super-nits.

Nits only, sorry for the delay -- typical waiting for r+ before sr+ problem.

>+const JSErrorFormatString* 
>+js_GetLocalizedErrorMessage (JSContext* cx, void *userRef, const char *locale, const uintN errorNumber)
>+{
>+    const JSErrorFormatString* errorString = NULL;

The * cuddles the declarator name, not the type name, in JS house style (K&R C).  Also, empty line after this declaration please.

>+    if (cx->localeCallbacks)
>+        errorString = (cx->localeCallbacks->localeGetErrorMessage) (userRef, locale, errorNumber);

No need to parenthesize cx->localeCallbacks->localeGetErrorMessage, and no space before ( before arg list.

>+    if (!errorString)
>+        errorString = js_GetErrorMessage (userRef, locale, errorNumber);

Ditto space before (.

>     *messagep = NULL;
>-    if (callback) {
>+    /* Most calls supply js_GetErrorMessage; if this is so, assume NULL. */

One empty line before this comment.

A diff -w (I applied the patch and extracted one myself) would show how minimal your change is -- recommended for any time you indent or unindent lots of code without changing it.

r+ with these nits picked.  Thanks very much for fixing this!

/be

Comment 19

[Michael Daumling]

Attachment: De-nitted patch

Patch is de-nitted. Should I use diff -w to create the patch as well?

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 204815 [details] [diff] [review]
De-nitted patch

>@@ -943,28 +930,30 @@ js_ErrorToException(JSContext *cx, const
> {
>     JSErrNum errorNumber;
>     JSExnType exn;
>     JSBool ok;
>     JSObject *errProto, *errObject;
>     JSString *messageStr, *filenameStr;
>     uintN lineno;
>     JSExnPrivate *privateData;
>+    const JSErrorFormatString* errorString;

Another case where the * should cuddle the declarator name.

But again, this is still r+ me -- should I go ahead and land it?

/be

Comment 21

[Michael Daumling]

Attachment: Mildly corrected patch

I forgot to declare js_GetLocalizedErrorMessage() in jsexn.h, which lead to a compiler warning. And I moved the * one space to the right :)

Brendan, should I use diff -w for future patches?

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 204819 [details] [diff] [review]
Mildly corrected patch

At this point, interdiff (there's UI for it at the top of the diff you get for the patch by clicking on the "diff" link) suffices, since I saved an earlier patch, applied it, and extracted my own -w.  But in general, with a patch that mostly or significantly reindents existing code, using -w is good.  To help revieweres, it's common to attach the -w after the regular unified diff.  Anyone wanting to patch the code needs the regular diff, though, so just a -w does not suffice.

/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: patch I'm checking in r=me crediting Michael

I cleaned up various comment, trailing whitespace, declarator style, space-before-paren-in-arg-list, etc. nits.  I also took the liberty, in advance of the JS_STRINGS_ARE_UTF8 => JS_C_STRINGS_ARE_UTF8 renaming, to rename the new API JS_StringsAreUTF8.  I hope that's ok.  The longer the old name lives in the API, the greater the odds of people using it.  We preserve API compatibility across releases, so it's important to get this right ASAP.

Astute readers will note an unrelated cleanup from my js.c, which I've been carrying too long.  The error reporter juggling in Load was all wrong, or at least, is now and has been for a long time (since exceptions?).  This fixes a silent failure bug.

/be

Comment 24

[Brendan Eich [:brendan]]

Attachment: real patch i'm checking in

Spent too long tracking down a bad impurity (double-free), which I though was a regression from this bug, but which is really a regression from the fix for bug 298286 (I'll file that when I get home in a bit).

/be

Comment 25

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 26

[tor]

This caused a crasher regression, bug 318873.

Comment 27

[Bob Clary [:bc] (inactive)]

js16 fixes should be in 1.8.1 as well me thinks.

Comment 28

[Mike Schroepfer]

MrBkap can you get a branch patch together for this?

Comment 29

[Brendan Eich [:brendan]]

(In reply to comment #28)
> MrBkap can you get a branch patch together for this?

We should take all of js1.7, which includes this fix, for 1.8.1.  I've talked to mrbkap about this.  Hand-merging pieces of js1.7 "features" and leaving out interleaved bug fixes is just going to make a configuration mess.  We've tested and designed what is in js1.7.  It's all or nothing.

/be

Comment 30

[Darin Fisher]

Not going to block FF2 beta1 for this bug.

Comment 31

[Bob Clary [:bc] (inactive)]

not going to make spidermonkey 1.6

Comment 33

[Mike Hommey [:glandium]]

Why did was it needed to change JS_StringsAreUTF8 to JS_CStringsAreUTF8 ?

Comment 34

[Braden]

Unfortunately, this is not completely fixed.

The exception *type* used for JS_ReportErrorNumber is still getting pulled from SpiderMonkey's js.msg. That is, when calling JS_ReportErrorNumber with the number of a user-specified exception, one gets an exception type corresponding to the same-numbered entry in js.msg.

In js_ErrorToException, it looks like the user's error callback is never checked when getting the exception type.

Comment 35

[Robin Edwards]

It might be worth noting this bug is still apparent in the javascript shell (js.c).

e.g. calling trap() or line2pc() with no parameters both generate a different kind of error, despite the fact that the supplied jsshell.msg suggests they should.

js> trap()
typein:1: InternalError: usage: trap [fun] [pc] expr
js> line2pc()
typein:2: TypeError: usage: line2pc [fun] line

jsshell.msg
MSG_DEF(JSSMSG_TRAP_USAGE,               2, 0, JSEXN_NONE, "usage:trap [fun][pc] expr")
MSG_DEF(JSSMSG_LINE2PC_USAGE,            3, 0, JSEXN_NONE, "usage:line2pc [fun] line")

Clearly the error type coming from js.msg-
MSG_DEF(JSMSG_INACTIVE,                 2, 0, JSEXN_INTERNALERR, " nothing active on context")
MSG_DEF(JSMSG_MORE_ARGS_NEEDED,         3, 3, JSEXN_TYPEERR, "{0} requires more than {1} argument{2}")

Comment 36

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Comment 37

[Till Schneidereit [:till]]

I just checked, and the remaining issue described in comment 34 and comment 35 is fixed.