290995 - Add Components.reportError()

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Benjamin Smedberg]

I'm working on advanced logging of JS errors, and I frequently have encountered
situations where a JS exception handler would like to report an exception to the
console, but then continue whatever it was doing.

To facilitate this, I have added a Components.reportError() method, which takes
a JS Error object (presumably from an exception handler) and reports it to the
console.

I have not security-protected this function, because you can flood the console
anyway (by throwing in a setTimeout, for example).

Comment 1

[Benjamin Smedberg]

Attachment: Add Components.reportError()

dbradley, would you like to review this, or shall I ask somebody else for the
second review?

Comment 2

[James Ross]

Comment on attachment 181161 [details] [diff] [review]
Add Components.reportError()

>+    // This function shall never fail! Silently eat any failure conditions.
>+    nsresult rv;

>+    if(argc < 1)
>+        return NS_ERROR_XPC_NOT_ENOUGH_ARGS;

The function seems to fail here; I presume you mean that the function will
never fail because of some other component.

Comment 3

[Benjamin Smedberg]

I mean, that it should not fail if called as designed. argc < 1 is a code error,
and it's fine to throw from that.

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 181161 [details] [diff] [review]
Add Components.reportError()

>+    // It's not a JS Error object, so we synthesize as best we're able
>+    JSString* msgstr = JS_ValueToString(cx, argv[0]);

You should root this here, in case something under scripterr->Init()
causes a GC:

  argv[0] = STRING_TO_JSVAL(msgstr);

Also, you should check that msgstr isn't null, which would
can indicate that you're being called on an object that
doesn't convert to string, or the usual OOM.

>+    rv = scripterr->Init(NS_REINTERPRET_CAST(const PRUnichar*,
>+                                             msgstr),

That cast looks totally wrong to me; does this actually work?

I think you want

NS_REINTERPRET_CAST(const PRUnichar *, JS_GetStringChars(msgstr))

and you want to check for NULL as well, unless Init is null-safe for its string
param.

>+                         nsnull,
>+                         nsnull,
>+                         0, 0,

We can get the current file/line info from nsXPConnect::GetCurrentJSStack,
which would make this report much more useful.

People who want to report a warning or a message can use the console service
directly, I guess.

Comment 5

[Benjamin Smedberg]

Attachment: Add Components.reportError(), rev. 1.1

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 181785 [details] [diff] [review]
Add Components.reportError(), rev. 1.1

>+    // get place for return value
>+    jsval *retval = nsnull;
>+    rv = cc->GetRetValPtr(&retval);
>+    if(NS_FAILED(rv) || !retval)
>+        return NS_OK;

You don't seem to use retval anywhere else, and I don't think there's
any interesting return value to give to the caller, so just lose this
section.

r+sr=shaver with that removal, requesting approval as well.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 181785 [details] [diff] [review]
Add Components.reportError(), rev. 1.1

s/if (/if(/ and open brace on next line to match prevailing style, and a=me.

/be

Comment 8

[Benjamin Smedberg]

Fixed on trunk.