1458731 - protocol.js include more descriptive errors

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Jason Laster [:jlast]]

It would be nice if errors included the actor id and stack.

Comment 1

[Jason Laster [:jlast]]

Attachment: Bug 1458731 - protocol.js include more descriptive errors.

Review commit: https://reviewboard.mozilla.org/r/241304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241304/

Comment 2

[Alexandre Poirot [:ochameau]]

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.

Comment 3

[Jason Laster [:jlast]]

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?

Comment 4

[Alexandre Poirot [:ochameau]]

(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 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1458731 - Print actor's typeName and name of the throwing method when an exception is thrown by a protocol.js's actor.

Review commit: https://reviewboard.mozilla.org/r/242678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242678/

Comment 6

[Alexandre Poirot [:ochameau]]

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

Comment 7

[Alexandre Poirot [:ochameau]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242678/diff/1-2/

Comment 8

[Jason Laster [:jlast]]

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.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7ab222f26391