827140 - Copying stack traces in the UI to the clipboard

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

If we implement the feature of copying stack trace, the bug report will be easy. And useful.

Comment 1

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

I won't be able to look at this very soon.

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: Copy stack to clipboard from context menu (first pass)

I'm expecting this patch will be rejected, because I'm extracting the stack trace from the menu items.  There has to be a cleaner way to get the full stack.

Comment 3

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

Comment on attachment 763177 [details] [diff] [review]
Copy stack to clipboard from context menu (first pass)

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

Hey! Thanks for the patch!

You can use the frameCache in StackFrames from DebuggerController, if you really want to, but then you'd have to go parsing urls and converting them to unicode all over again. So I advise against that. However, as you might have guessed, using the menu items is not that nice. See below.

Just need to add a test and address some minor issues, other than that, f+.

::: browser/devtools/debugger/debugger-controller.js
@@ +590,5 @@
>      for (let frame of this.activeThread.cachedFrames) {
>        this._addFrame(frame);
>      }
> +
> +    DebuggerView.StackFrames.addCopyCommand();

Could you make this operation the last one please?

::: browser/devtools/debugger/debugger-toolbar.js
@@ +443,5 @@
>  
>      this._framesCache.set(aDepth, stackframeItem);
>    },
>  
> +  getStack: function() {

Might want to rename this to "getStackAsString". Please document this method as well.

@@ +444,5 @@
>      this._framesCache.set(aDepth, stackframeItem);
>    },
>  
> +  getStack: function() {
> +    // XXX ajvincent There has to be a better way to do this than iterating over menuitem elements...

Yup.

in _addFrame, when you append a stack frame item to this container, add the following information:

this.push(...
  attachment: {
    sourceLocation: aSourceLocation,
    lineNumber: aLineNumber,
    ...
  },
  ...
});

And here:

var report = "";
for (let frameItem in this) {
  let { depth, sourceLocation, lineNumber }
  report += depth + ". " + sourceLocation + ": " + lineNumber + "\n";
}

@@ +454,5 @@
> +      report += msg + "\n";
> +    }
> +    return report;
> +  },
> +  

Nit: trailing whitespace.

@@ +455,5 @@
> +    }
> +    return report;
> +  },
> +  
> +  addCopyCommand: function() {

Please document this function.

@@ +459,5 @@
> +  addCopyCommand: function() {
> +    this._menupopup.appendChild(document.createElement("menuseparator"));
> +    let copyItem = document.createElement("menuitem");
> +    copyItem.setAttribute("label", "Copy");
> +    let _this = this;

I haven't seen this _this for a while! There's a better way!

copyItem.addEventListener("command", () => {
  let s = this.getStack();
  ...
});

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/arrow_functions#Lexical_this

@@ +462,5 @@
> +    copyItem.setAttribute("label", "Copy");
> +    let _this = this;
> +    copyItem.addEventListener("command", function() {
> +      let s = _this.getStack();
> +      const gClipboardHelper = Components.classes["@mozilla.org/widget/clipboardhelper;1"]

Why const? Why gClipboardHelper, since it's not a global? This would be better:

let clipboardHelper = Cc["@mozilla.org/widget/clipboardhelper;1"]
  .getService(Ci.nsIClipboardHelper)

@@ +466,5 @@
> +      const gClipboardHelper = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
> +                                         .getService(Components.interfaces.nsIClipboardHelper);
> +      gClipboardHelper.copyString(s);
> +    }, false);
> +    this._menupopup.appendChild(copyItem);

Is this menuitem ever removed? Does it appear twice when you get a fresh stack?

Comment 4

[Alex Vincent [:WeirdAl]]

[10:42]	victorporof	WeirdAl: see browser_dbg_aaa_run_first_leaktest.js
[10:42]	victorporof	it's a barebones test that does nothing
[10:42]	victorporof	but it gets you a stacktrace!
[10:42]	victorporof	clone it and add your stuff in the performTest callback

Comment 5

[Rob Campbell [:rc] (:robcee)]

no activity for > 6mos. Alex, reassign yourself if you see yourself working on this again soon. Otherwise, someone else should finish this.

Comment 8

[Hubert B Manilla]

I'm working on this. A quick question.
What context menu would we ideally want to put the copy command unto?
ATM i'm creating a context menu for the Stack frames view which would have the copy command, is that the best place for it?

Comment 9

[Hubert B Manilla]

Attachment: 827140wip.patch

What context menu would we ideally want to put the copy command unto?
ATM i've created a context menu for the Stack frames view which would have the copy command, is that the best place for it?

Comment 10

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

Comment on attachment 8629932 [details] [diff] [review]
827140wip.patch

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

Looks good. But it's gonna need a test. Let me know if you need any help with that.

This place is good for the context menu. It doesn't prevent us from adding it to different places too in the future.

Comment 11

[Hubert B Manilla]

Attachment: bug827140.patch

I took a stab at the tests.

i'm having an issue with the second test (browser/devtools/debugger/test/browser_dbg_stack-contextmenu-02.js), i seem to have hit a block.

i get this

21 INFO TEST-UNEXPECTED-FAIL | browser/devtools/debugger/test/browser_dbg_stack-contextmenu-02.js | Timed out while polling clipboard for pasted data -
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:947
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard/wait/<:964
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:EventLoop.prototype.enter:339
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:ThreadActor.prototype._pushThreadPause:534
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:ThreadActor.prototype._pauseAndRespond:735
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:ThreadActor.prototype.onDebuggerStatement:1795
http://example.com/browser/browser/devtools/debugger/test/doc_recursion-stack.html:simpleCall:14
chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:this.call:18
chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:null:71
null:null:0

i'm assuming it can't find the string copied to the clipboard...but not sure.
Would appreciate help here. Thanks

Comment 12

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

Comment on attachment 8636715 [details] [diff] [review]
bug827140.patch

Please ask :jlongster for feedback/review since he's now the debugger maintainer.

Comment 13

[James Long (:jlongster)]

Comment on attachment 8636715 [details] [diff] [review]
bug827140.patch

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

I'm assuming you are having problems because your `STACK_STRING` did not have a newline at the end, and the code which gets the stack as a string was always appending a newline to each line, even the last one.

::: browser/devtools/debugger/views/stack-frames-view.js
@@ +116,5 @@
> +    var report = "";
> +    for (let frameItem of this) {
> +      let { attachment: { depth, url, line }} = frameItem;
> +      report += depth + ". " + url + ": " + line + "\n";
> +    }

You are always adding a newline add the end. Do this instead:

return [...this].map(frameItem => {
  const { attachment: { depth, url, line }} = frameItem;
  return depth + ". " + url + ": " + line;
}).join("\n");

I might have an error in there, but you get the idea.

Comment 14

[Hubert B Manilla]

:jlongster thanks.

Comment 15

[Hubert B Manilla]

Attachment: bug827140.patch

Comment 16

[Hubert B Manilla]

Attachment: bug827140.patch

Comment 17

[(not reading bugmail) Nick Desaulniers [:\n]]

+1

When paused in the debugger, when viewing the call stack, I'd like to be able to somehow copy the call stack into the copy buffer at any given step/break point.  

For example, I'm debugging my code, hit a break point, step around, view the stack trace tab, right click + copy call stack, paste the call stack into an email to a fellow developer.

The contents of the call stack might be equivalent to `(new Error).stack`, from where the debugger is currently paused (file, line, col, fn name).

Comment 18

[James Long (:jlongster)]

Comment on attachment 8657259 [details] [diff] [review]
bug827140.patch

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

Looks great! A few nits about the format: is it really useful to include the depth information here? I think most people would be surprised by that. Nick what do you think? Right now the stack on Error objects is a little different than the stack we have in the debugger, and I don't think we can get something like "function name" right now. For now, at least having "url:line:column" is a good start.

I think we should follow the Error stack format: remove the space between url and line, and make sure to add column in there. It should just be another property on the frame object. r+ if you do this, or convince me differently :)

Comment 19

[(not reading bugmail) Nick Desaulniers [:\n]]

(In reply to James Long (:jlongster) from comment #18)
> Nick
> what do you think? Right now the stack on Error objects is a little
> different than the stack we have in the debugger, and I don't think we can
> get something like "function name" right now.

I mean, the function names are listed in the UI/DOM elements created within the debugger under the stack trace tab.  Couldn't their contents just be iterated and copied into the copy buffer?  I could file a separate bug as a follow up?

Comment 20

[James Long (:jlongster)]

(In reply to Nick Desaulniers [:\n] from comment #19)
> (In reply to James Long (:jlongster) from comment #18)
> > Nick
> > what do you think? Right now the stack on Error objects is a little
> > different than the stack we have in the debugger, and I don't think we can
> > get something like "function name" right now.
> 
> I mean, the function names are listed in the UI/DOM elements created within
> the debugger under the stack trace tab.  Couldn't their contents just be
> iterated and copied into the copy buffer?  I could file a separate bug as a
> follow up?

Oh, you're totally right.

Hubert, can you change the format to the following?

  const { attachment: { title, url, line, column }} = frameItem;
  return title + '@' + url + ":" + line + ':' + column;

That `title` is what the view made for the frame, which is just whatever "display name" the Debugger.Frame object has, which is likely just the function name.

The `column` is not actually saved in the `frameItem` right now, so you'll also need to add it here: https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/debugger/views/stack-frames-view.js#L100. Just add a `column: aFrame.column` property.

Comment 21

[Hubert B Manilla]

Attachment: bug827140.patch

Comment 22

[Hubert B Manilla]

(In reply to James Long (:jlongster) from comment #20)
> (In reply to Nick Desaulniers [:\n] from comment #19)
> > (In reply to James Long (:jlongster) from comment #18)
> > > Nick
> > > what do you think? Right now the stack on Error objects is a little
> > > different than the stack we have in the debugger, and I don't think we can
> > > get something like "function name" right now.
> > 
> > I mean, the function names are listed in the UI/DOM elements created within
> > the debugger under the stack trace tab.  Couldn't their contents just be
> > iterated and copied into the copy buffer?  I could file a separate bug as a
> > follow up?
> 
> Oh, you're totally right.
> 
> Hubert, can you change the format to the following?
> 
>   const { attachment: { title, url, line, column }} = frameItem;
>   return title + '@' + url + ":" + line + ':' + column;
> 
> That `title` is what the view made for the frame, which is just whatever
> "display name" the Debugger.Frame object has, which is likely just the
> function name.
> 
> The `column` is not actually saved in the `frameItem` right now, so you'll
> also need to add it here:
> https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/debugger/
> views/stack-frames-view.js#L100. Just add a `column: aFrame.column` property.

i've made the neccessary changes

Comment 23

[James Long (:jlongster)]

Looks good, sorry it took so long to get around to this again. I just applied it locally and everything works. Let's land this.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9860b9cd1653

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9860b9cd1653

Comment 26

[Hubert B Manilla]

Nice! Thanks :jlongster for your help.

Comment 27

[Mayur Patil]

[bugday-20160323]

STR:
1. Tools -> Options -> Web Developer -> Debugger
2. Debugger will open. It is having two sub tabs: Sources and Call stack.
3. Now pause debugger.
4. Go to js window and js code.
5. Paste it into Email for more debugging.

Status: RESOLVED,FIXED --> VERIFIED

Component: 

Name 	        Firefox
Version 	46.0b2
Build ID 	20160314144540
Update Channel 	beta
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS              Windows 7 SP1 x86_64

Actual Results: 
As expected

Expected Results: 
Able to copy stack trace into copy buffer and able send it to developer.

Comment 28

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I verified this bug has been fixed. Thank you!