Closed Bug 960905 Opened 6 years ago Closed 6 years ago

DevToolsUtils.reportException is misused in Tracer.prototype

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: vporof, Assigned: euler90h)

Details

(Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js])

Attachments

(2 files)

|reportException| takes two arguments.
Maybe we should just use console.exception everywhere.
I think that the work that safeErrorString does is valuable. Does console.exception stringify errors with their stack (and soon line and column numbers) as well has handle non error objects? We could make sure it does.

Alternatively, I think it would make sense to make reportException's string argument optional and make the error object the first parameter.
To whoever picks this up, reportException needs the first argument to be a string like "Tracer.prototype.onEnter" and the second argument should be the error. Right now it looks like we are only passing the error objects.

This needs to be fixed in the methods defined on Tracer.prototype: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1455

See also: https://wiki.mozilla.org/DevTools/Hacking
Priority: -- → P3
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
I may be able to fix this bug. Just to make sure I'm understanding this, for instance inside the method Tracer.startTracing on line 1511 of debugger-controller.js, it says
   DevToolsUtils.reportException(error);
but we want it to say
   DevToolsUtils.reportException("Tracer.startTracing", error);
is this correct?
Pretty much, although it would be

  DevToolsUtils.reportException("Tracer.prototype.startTracing", error);
Assignee: nobody → euler90h
Status: NEW → ASSIGNED
Here's the patch, sorry for the wait! Also, I noticed that DevToolsUtil.reportException is misused in the same way in the file /toolkit/devtools/server/actors/script.js on line 4712. If you wish, I can upload a patch fixing that as well.
Fixes the misuse described in the comment to my first patch.
Attachment #8383838 - Flags: review?(nfitzgerald)
Attachment #8383826 - Flags: review?(nfitzgerald)
Comment on attachment 8383826 [details] [diff] [review]
Patch fixing bug 960905

Review of attachment 8383826 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me :)
Attachment #8383826 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8383838 [details] [diff] [review]
bug960905_2.patch

Review of attachment 8383838 [details] [diff] [review]:
-----------------------------------------------------------------

Yup!
Attachment #8383838 - Flags: review?(nfitzgerald) → review+
https://hg.mozilla.org/mozilla-central/rev/d7f8049ca57c
https://hg.mozilla.org/mozilla-central/rev/aff6c68092f5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.