If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add Components.reportError()

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
XPConnect
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla1.8beta2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

13 years ago
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

13 years ago
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?
Attachment #181161 - Flags: review?(shaver)

Comment 2

13 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

13 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

13 years ago
Created attachment 181785 [details] [diff] [review]
Add Components.reportError(), rev. 1.1
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 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

13 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta2
You need to log in before you can comment on or make changes to this bug.