Closed
Bug 290995
Opened 19 years ago
Closed 19 years ago
Add Components.reportError()
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla1.8beta2
People
(Reporter: benjamin, Assigned: benjamin)
Details
Attachments
(1 file, 1 obsolete file)
7.22 KB,
patch
|
shaver
:
review+
shaver
:
superreview+
brendan
:
approval1.8b2+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Comment 1•19 years ago
|
||
dbradley, would you like to review this, or shall I ask somebody else for the second review?
Attachment #181161 -
Flags: review?(shaver)
Comment 2•19 years ago
|
||
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.
Assignee | ||
Comment 3•19 years ago
|
||
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[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.
Attachment #181161 -
Flags: review?(shaver) → review-
Assignee | ||
Comment 5•19 years ago
|
||
Attachment #181161 -
Attachment is obsolete: true
Attachment #181785 -
Flags: review?(shaver)
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.
Attachment #181785 -
Flags: superreview+
Attachment #181785 -
Flags: review?(shaver)
Attachment #181785 -
Flags: review+
Attachment #181785 -
Flags: approval1.8b2?
Comment 7•19 years ago
|
||
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
Attachment #181785 -
Flags: approval1.8b2? → approval1.8b2+
Assignee | ||
Comment 8•19 years ago
|
||
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta2
You need to log in
before you can comment on or make changes to this bug.
Description
•