960905 - DevToolsUtils.reportException is misused in Tracer.prototype

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

|reportException| takes two arguments.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Maybe we should just use console.exception everywhere.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 4

[Stefan]

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?

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Pretty much, although it would be

  DevToolsUtils.reportException("Tracer.prototype.startTracing", error);

Comment 6

[Stefan]

Attachment: 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.

Comment 7

[Stefan]

Attachment: bug960905_2.patch

Fixes the misuse described in the comment to my first patch.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8383826 [details] [diff] [review]
Patch fixing bug 960905

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

Looks good to me :)

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8383838 [details] [diff] [review]
bug960905_2.patch

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

Yup!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d7f8049ca57c
https://hg.mozilla.org/integration/fx-team/rev/aff6c68092f5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7f8049ca57c
https://hg.mozilla.org/mozilla-central/rev/aff6c68092f5