1430022 - Copy command on cyclic object should display an error message in the console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce:
1. Open the webconsole
2. Evaluate the following code : `var a = {};a.b = a; copy(a);`


Expected results:

An error is displayed in the console, as if I was doing `var a = {};a.b = a; JSON.stringify(a);` ("TypeError: cyclic object value[Learn More]")

Actual results:

There is nothing displayed in the console, and when I try to paste the result of the copy command I get "/* TypeError: cyclic object value */".

This is not useful because : 
- I don't have immediate feedback (and I may already have switched to my editor to paste the thing I wanted to copy, and thus I have to go back to the console, and figure out what is going on)
- I don't have the "Learn more" link that could tell me what this error means.

Comment 1

[Brian Grinstead [:bgrins]]

Good idea, showing an error in the console would be much better than putting this on the clipboard

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

So, here is where we put the error into a comment: https://searchfox.org/mozilla-central/rev/137f1b2f434346a0c3756ebfcbdbee4069e15dc8/devtools/server/actors/webconsole/utils.js#587

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1430022 - [WIP] Better error handling on copy command;

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

The patch from Comment 3 is already better than the current situation, but: 
- The error does not say what caused it. We should have something that say: "Error while copying object: {the actual exception}".
- The error is not l10n'ed
- There is no "learn more" link to make sense of the error.

Also, the error is not shown if you do the copy via the context menu entry (right click on object, and select "Copy object").

We should fire the same message as if you were doing `x = {}; x.y = x;JSON.stringify(x)`, only with a prefix to indicate it is a result of the copy command.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8942612 [details]
Bug 1430022 - [WIP] Better error handling on copy command;

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

Comment 6

[Abhinav Raj]

@Nicholas can you please assign me this task, I am working on it

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

sure! thanks

Comment 8

[Abhinav Raj]

Attachment: Bug 1430022 -Added error message in copy command on cyclic object value. r=Nicolas Chevobbe

Bug 1430022 -Added error message in copy command on cyclic object value

Comment 9

[Abhinav Raj]

Attachment: Bug 1430022: Removed try catch block

Comment 10

[Abhinav Raj]

@Nicolas by Mistake I have opened another PR, please ignore that, I will update the original commit

Comment 11

[Mark Banner (:standard8)]

(In reply to Abhinav Raj from comment #10)

@Nicolas by Mistake I have opened another PR, please ignore that, I will update the original commit

Please can you abandon the extra revision? If you visit it at https://phabricator.services.mozilla.com/D62046, the scroll down to the bottom. Just above the comments box, there's an Add Action option... select Abandon revision from that, then hit submit.

Comment 12

[Abhinav Raj]

(In reply to Mark Banner (:standard8) from comment #11)

(In reply to Abhinav Raj from comment #10)

@Nicolas by Mistake I have opened another PR, please ignore that, I will update the original commit

Please can you abandon the extra revision? If you visit it at https://phabricator.services.mozilla.com/D62046, the scroll down to the bottom. Just above the comments box, there's an Add Action option... select Abandon revision from that, then hit submit.

Done

Comment 13

[Mark Banner (:standard8)]

(In reply to Abhinav Raj from comment #12)

(In reply to Mark Banner (:standard8) from comment #11)

(In reply to Abhinav Raj from comment #10)

@Nicolas by Mistake I have opened another PR, please ignore that, I will update the original commit

Please can you abandon the extra revision? If you visit it at https://phabricator.services.mozilla.com/D62046, the scroll down to the bottom. Just above the comments box, there's an Add Action option... select Abandon revision from that, then hit submit.

Done

Hmm, I don't see any changes, it still looks like it is open. Did you hit submit after selecting abandon revision?

Comment 14

[Mark Banner (:standard8)]

That's got it, thank you!

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Abhinav, are you still working on this?

Comment 16

[Alex]

@nchevobbe I can take a look into this bug.

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Thanks Alex, the bug is now yours :)

What needs to be done:

• In devtools/server/actors/webconsole/utils.js#620 , we don't want to assign the exception to the payload, but return an object that will indicate to the client that there was an error, like we do in devtools/server/actors/webconsole/utils.js#567-570
• add a localized string like we have in devtools/client/locales/en-US/webconsole.properties#101
• add a test case for this, in the already existing test devtools/client/webconsole/test/browser/browser_jsterm_copy_command.js

it would be nice to still pass the exception message to the client, and try to display it in the error message, but let's focus on having a message displayed first :)

Comment 18

[Claudia Batista [:claubatista]]

Hi! I'd like to work on this. :D

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

thanks for helping us Claudia, the bug is now yours :)

Comment 20

[Claudia Batista [:claubatista]]

Attachment: Bug 1430022 - [devtools] Adds an error message in the copy command on a cyclic object r=nchevobbe

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55ab12720df1
[devtools] Adds an error message in the copy command on a cyclic object r=nchevobbe

Comment 22

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/55ab12720df1