DevToolsUtils.reportException is misused in Tracer.prototype

RESOLVED FIXED in Firefox 30

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vporof, Assigned: Stefan)

Tracking

unspecified
Firefox 30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
|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]
(Assignee)

Comment 4

4 years ago
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
(Assignee)

Comment 6

4 years ago
Created attachment 8383826 [details] [diff] [review]
Patch fixing bug 960905

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.
(Assignee)

Comment 7

4 years ago
Created attachment 8383838 [details] [diff] [review]
bug960905_2.patch

Fixes the misuse described in the comment to my first patch.
Attachment #8383838 - Flags: review?(nfitzgerald)
(Assignee)

Updated

4 years ago
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+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/d7f8049ca57c
https://hg.mozilla.org/integration/fx-team/rev/aff6c68092f5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d7f8049ca57c
https://hg.mozilla.org/mozilla-central/rev/aff6c68092f5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30

Updated

4 years ago
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.