286450 - nsScriptError.message is not very helpful

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Benjamin Smedberg]

nsScriptError implements nsIConsoleMessage.message as the plain error message
without line# etc. Instead, nsIConsoleMessage.message should return the same
thing as nsIScriptError.toString, and there should be a separate attribute on
nsIScriptError to access the "bare" error message. Patch coming up.

I need this for error logging to file in xulrunner/toolkit startup.

Comment 1

[Benjamin Smedberg]

Attachment: Add nsIScriptError.errorMessage

Comment 2

[Darin Fisher]

Don't forget to change the interface UUID!

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 177633 [details] [diff] [review]
Add nsIScriptError.errorMessage

you should check for OOM from ToNewUnicode before returning NS_OK; but why add
a method that's redundant with toString?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, what biesi said.. having two methods/properties that do the same exact
thing seems a little odd.  Why not just use toString() in whatever code it is
you have?

Comment 5

[Benjamin Smedberg]

Because my logging code is XPCOM-only (it does not and cannot know about
nsIScriptError). Basically, this makes the nsIConsoleMessage interface output a
reasonably helpful error message even without knowing anything about nsIScriptError.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177633 [details] [diff] [review]
Add nsIScriptError.errorMessage

Ah, I see.

>Index: js/src/xpconnect/idl/nsIScriptError.idl

Like darin said, rev the uuid.

>+    /**
>+     * The error message without any context/line number information.
>+     */
>+    readonly attribute wstring errorMessage;

Probably worth more clearly documenting how this differs from the "message"
attribute...

Also, possibly worth making this an AString, not a wstring.

>Index: js/src/xpconnect/src/nsScriptError.cpp

>+    nsXPIDLCString message;
>+    rv = ToString(getter_Copies(message));

So now "message" is in what encoding?  Looking at ToString(), it's just taking
the UTf-16 string and dropping the high bytes...

>+    *result = ToNewUnicode(message);

And then we byte-expand.  So ISO-8859-1 will survive this procedure, but
everything else is screwed.

I'd sort of prefer something that didn't do that (perhaps use ToNewUTF8String
in toString() and then UTFtoNewUnicode, or CopyUTF8ToUTF16 depending on what
the attr ends up returning here?).

Comment 7

[Benjamin Smedberg]

Attachment: Add nsIScriptError.errorMessage and use new string types

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177865 [details] [diff] [review]
Add nsIScriptError.errorMessage and use new string types

Much happier!  sr=bzbarsky.

Comment 9

[David Bradley]

Comment on attachment 177865 [details] [diff] [review]
Add nsIScriptError.errorMessage and use new string types

r=dbradley

Comment 10

[Benjamin Smedberg]

Fixed on trunk.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

js/src/xpconnect/src/xpcconvert.cpp
+        data->ToString(formattedMsg);
 
         rv = ConstructException(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS,
-                                formattedMsg, ifaceName, methodName, data,
+                                formattedMsg.get(), ifaceName, methodName, data,

Does this use the correct charset?

Comment 12

[Simon Paquet [:sipaq]]

Comment on attachment 177633 [details] [diff] [review]
Add nsIScriptError.errorMessage

patch is obsolete, removing review request