Closed Bug 955273 Opened 6 years ago Closed 6 years ago

Errors in handlers lose their location information

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 1840 at 2012-11-28 17:14:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1840 as attmnt 2142 at 2012-11-28 17:14:00 UTC ***

Adds an optional parameter to scriptError() to which a caught error can be passed. In this case, the location information from this error is used instead of that of the exception handler. The original error message is appended to the new one.
Attachment #8353904 - Flags: review?(florian)
Attachment #8353904 - Flags: feedback?(clokep)
*** Original post on bio 1840 at 2012-11-28 17:18:23 UTC ***

I guess my first question is whether the text we're adding really adds more information or if we could just show the original error. (I.e. does it need to be an extra parameter or can we just pass it into this.error?)
*** Original post on bio 1840 at 2012-11-28 17:22:50 UTC ***

(In reply to comment #1)
> I guess my first question is whether the text we're adding really adds more
> information or if we could just show the original error. (I.e. does it need to
> be an extra parameter or can we just pass it into this.error?)
This patch is intended as an improvement over just adding "+ e" to the end of the new error string (look at the IRC use case in the patch) As we have to pass e as a parameter for the location information anyway, we don't have to add "+ e" to the string anymore by hand either.
*** Original post on bio 1840 at 2012-11-28 22:48:06 UTC ***

(In reply to comment #1)
> I guess my first question is whether the text we're adding really adds more
> information or if we could just show the original error. (I.e. does it need to
> be an extra parameter or can we just pass it into this.error?)

Are you asking if we should add a check to detect when the value passed as aMessage parameter is actually already an nsIScriptError instance, and then use that instead of creating a new one?

If that's your question, then I think it could be useful, but I don't think it solves the problem aleth is trying to solve here.
Comment on attachment 8353904 [details] [diff] [review]
Patch

*** Original change on bio 1840 attmnt 2142 at 2012-11-28 22:54:06 UTC ***

>-  scriptError.init(aMessage, caller.filename, sourceLine, caller.lineNumber,
>-                   null, flag, "component javascript");
>+  if (!aOriginalError) {
>+    scriptError.init(aMessage, caller.filename, sourceLine, caller.lineNumber,
>+                     null, flag, "component javascript");
>+  }
>+  else {
>+    aMessage += "\n" + (aOriginalError.message || aOriginalError);
>+    scriptError.init(aMessage, aOriginalError.fileName || caller.filename,
>+                     sourceLine, aOriginalError.lineNumber || caller.lineNumber,
>+                     null, flag, "component javascript");
>+  }

This triggers my usual "code duplication!" warning ;).

I think I would prefer it written this way:

  let fileName = caller.filename;
  let lineNumber = caller.lineNumber;
  if (aOriginalError) {
    aMessage += "\n" + (aOriginalError.message || aOriginalError);
    if (aOriginalError.fileName)
      fileName = aOriginalError.fileName;
    if (aOriginalError.lineNumber)
      lineNumber = aOriginalError.lineNumber
  }
  let scriptError =
    Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
  scriptError.init(...)
Attachment #8353904 - Flags: review?(florian) → review-
*** Original post on bio 1840 at 2012-11-28 22:56:36 UTC ***

Comment on attachment 8353904 [details] [diff] [review] (bio-attmnt 2142)
Patch

>-function scriptError(aModule, aLevel, aMessage) {
>+function scriptError(aModule, aLevel, aMessage, aOriginalError) {

Mind adding a comment explaining what the parameters are?
Comment on attachment 8353904 [details] [diff] [review]
Patch

*** Original change on bio 1840 attmnt 2142 at 2012-11-29 11:45:16 UTC ***

(In reply to comment #3)
> (In reply to comment #1)
> > I guess my first question is whether the text we're adding really adds more
> > information or if we could just show the original error. (I.e. does it need to
> > be an extra parameter or can we just pass it into this.error?)
> 
> Are you asking if we should add a check to detect when the value passed as
> aMessage parameter is actually already an nsIScriptError instance, and then use
> that instead of creating a new one?
> 
> If that's your question, then I think it could be useful, but I don't think it
> solves the problem aleth is trying to solve here.

Yes, I was trying to understand what aleth was solving, he explained it online and I'm fine with these changes. (Plus florian's nits. :))
Attachment #8353904 - Flags: feedback?(clokep) → feedback+
Attached patch PatchSplinter Review
*** Original post on bio 1840 as attmnt 2143 at 2012-11-29 15:24:00 UTC ***

Feel free to amend the comment I added if you see a way to improve it.
Attachment #8353905 - Flags: review?(florian)
Comment on attachment 8353904 [details] [diff] [review]
Patch

*** Original change on bio 1840 attmnt 2142 at 2012-11-29 15:24:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353904 - Attachment is obsolete: true
Comment on attachment 8353905 [details] [diff] [review]
Patch

*** Original change on bio 1840 attmnt 2143 at 2012-12-14 23:26:33 UTC ***

I tweaked the comment a bit, mostly to make it look like a doxygen comment.
Attachment #8353905 - Flags: review?(florian) → review+
*** Original post on bio 1840 at 2012-12-14 23:32:54 UTC ***

Fixed in http://hg.instantbird.org/instantbird/rev/c440a7324cd1

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.