protocol.js include more descriptive errors

RESOLVED FIXED in Firefox 62

Status

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: jlast, Assigned: ochameau)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

Last year
It would be nice if errors included the actor id and stack.
Comment hidden (mozreview-request)
Assignee

Comment 2

Last year
mozreview-review
Comment on attachment 8972752 [details]
Bug 1458731 - protocol.js include more descriptive errors.

https://reviewboard.mozilla.org/r/241304/#review247250

::: devtools/shared/protocol.js:1006
(Diff revision 1)
>  
>    writeError: function(error) {
>      console.error(error);
>      if (error.stack) {
> -      dump(error.stack);
> +      dump(`Error in ${this.actorID} ${error.message}\n${error.stack}\n`);
>      }

It would be great to understand why you are not seeing the `console.error(error)` call.

If I add a `throw new Error("foo")` in actors/frame.js's getEnvironment, I get a correct error message:
```
0:15.96 GECKO(57300) console.error: (new Error("foo", "resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/frame.js", 53))
 0:15.96 GECKO(57300) getEnvironment@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/frame.js:53:11
 0:15.96 GECKO(57300) handler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1141:19
 0:15.96 GECKO(57300) onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1761:15
 0:15.96 GECKO(57300) receiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:735:7
 0:15.96 GECKO(57300) enter@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/utils/event-loop.js:118:5
 0:15.96 GECKO(57300) _pushThreadPause@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:178:5
 0:15.96 GECKO(57300) _pauseAndRespond@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:390:7
 0:15.97 GECKO(57300) async*onDebuggerStatement@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:1516:9
 0:15.97 GECKO(57300) @debugger eval code:1:1
```

As-is, your patch is going to duplicate the error message. As `error.message` is displayed in the `console.log` call.
Attachment #8972752 - Flags: review?(poirot.alex)
Severity: normal → enhancement
Priority: -- → P3
Reporter

Comment 3

Last year
hmm, that is a great point Alex, i wouldnt want to make it worse.

2 questions:

1. do you think that console.error could now be obsolete?
2. how would you search for how console.error should behave?
(In reply to Jason Laster [:jlast] from comment #3)
> hmm, that is a great point Alex, i wouldnt want to make it worse.
> 
> 2 questions:
> 
> 1. do you think that console.error could now be obsolete?

No. console.error should be fine and it is fine when using m-c ./mach mochitest. We can see both dump and console calls.

> 2. how would you search for how console.error should behave?

console prints on stdout from c++ right here:
  https://searchfox.org/mozilla-central/source/dom/console/Console.cpp#2965-2980
But this is only done if browser.dom.window.dump.enabled is set to true or the firefox build is a debug one.
Comment on attachment 8974343 [details]
Bug 1458731 - Print actor's typeName and name of the throwing method when an exception is thrown by a protocol.js's actor.

I got into this today.

When you specify "json" in specs and includes an Actor in the response, you will get this exception:
console.error: (new TypeError("cyclic object value", "resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js", 775))
send@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:775:9
send@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1476:5
sendReturn@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1184:11
promise callback*handler/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1188:18
_queueResponse@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1029:20
handler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1187:9
onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1761:15
receiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:735:7

That's because the actor has cyclic values and can't be converted to JSON via JSON.stringify.
But the current code doesn't help knowing which actor, nor which method throws.

Here is a patch to print the actor's typeName and the method name:
console.error: "Error while calling actor 'netEvent's method 'getResponseContent'" "cyclic object value"
send@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:775:9
send@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1476:5
sendReturn@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1184:11

I converted the call to dump into console.error as I don't see why we are using dump here.
Jason, let me know if that helps debugging issues from debugger? The switch to console.error may make all errors to disappear from debugger tests output...
Attachment #8974343 - Flags: feedback?(jlaster)
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Attachment #8974343 - Flags: feedback?(jlaster)
Reporter

Comment 8

Last year
mozreview-review
Comment on attachment 8974343 [details]
Bug 1458731 - Print actor's typeName and name of the throwing method when an exception is thrown by a protocol.js's actor.

https://reviewboard.mozilla.org/r/242678/#review248574

I haven't tested it, but the code looks good. r+ from me if you can see a typeError in the mochitest logs.
Attachment #8974343 - Flags: review+

Comment 9

Last year
mozreview-review
Comment on attachment 8974343 [details]
Bug 1458731 - Print actor's typeName and name of the throwing method when an exception is thrown by a protocol.js's actor.

https://reviewboard.mozilla.org/r/242678/#review250098

Great, seems like another helpful message!
Attachment #8974343 - Flags: review?(jryans) → review+
Assignee

Updated

Last year
Attachment #8974343 - Flags: feedback?(jlaster)
Assignee

Updated

Last year
Assignee: nobody → poirot.alex

Comment 10

Last year
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ab222f26391
Print actor's typeName and name of the throwing method when an exception is thrown by a protocol.js's actor. r=jlast,jryans
https://hg.mozilla.org/mozilla-central/rev/7ab222f26391
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Assignee

Updated

Last year
Blocks: dt-pjs

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.