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).
Created attachment 181161 [details] [diff] [review] Add Components.reportError() dbradley, would you like to review this, or shall I ask somebody else for the second review?
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.
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 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); You should root this here, in case something under scripterr->Init() causes a GC: argv = 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.
Created attachment 181785 [details] [diff] [review] Add Components.reportError(), rev. 1.1
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 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
Fixed on trunk.