Copying stack traces in the UI to the clipboard

VERIFIED FIXED in Firefox 46

Status

()

Firefox
Developer Tools: Debugger
P3
normal
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: tetsuharu, Assigned: Hubert B Manilla, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [lang=js][lang=xul])

Attachments

(2 attachments, 4 obsolete attachments)

If we implement the feature of copying stack trace, the bug report will be easy. And useful.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P3
I won't be able to look at this very soon.
Assignee: vporof → nobody
Status: ASSIGNED → NEW
Created attachment 763177 [details] [diff] [review]
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.
Assignee: nobody → ajvincent
Attachment #763177 - Flags: review?(vporof)
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?
Attachment #763177 - Flags: review?(vporof) → feedback+
[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
no activity for > 6mos. Alex, reassign yourself if you see yourself working on this again soon. Otherwise, someone else should finish this.
Assignee: ajvincent → nobody
Whiteboard: [mentor=vporof][lang=js][lang=xul]
Mentor: vporof@mozilla.com
Whiteboard: [mentor=vporof][lang=js][lang=xul] → [lang=js][lang=xul]

Updated

3 years ago
Duplicate of this bug: 1054329
Duplicate of this bug: 1066782
Summary: Enable to copying stack trace → Enable to copying stack trace / call stack
Summary: Enable to copying stack trace / call stack → Enable copying stack trace / call stack

Updated

3 years ago
Summary: Enable copying stack trace / call stack → Copying stack traces to the clipboard

Updated

3 years ago
Summary: Copying stack traces to the clipboard → Copying stack traces in the UI to the clipboard

Updated

3 years ago
Blocks: 1074592
(Assignee)

Comment 8

2 years ago
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?
(Assignee)

Comment 9

2 years ago
Created attachment 8629932 [details] [diff] [review]
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?
Attachment #8629932 - Flags: feedback?(vporof)
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.
Attachment #8629932 - Flags: feedback?(vporof) → feedback+
(Assignee)

Comment 11

2 years ago
Created attachment 8636715 [details] [diff] [review]
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
Attachment #8629932 - Attachment is obsolete: true
Attachment #8636715 - Flags: feedback?(vporof)

Updated

2 years ago
Assignee: nobody → b4bomsy
Comment on attachment 8636715 [details] [diff] [review]
bug827140.patch

Please ask :jlongster for feedback/review since he's now the debugger maintainer.
Attachment #8636715 - Flags: feedback?(vporof)
(Assignee)

Updated

2 years ago
Flags: needinfo?(jlong)
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.
Flags: needinfo?(jlong)
(Assignee)

Comment 14

2 years ago
:jlongster thanks.
(Assignee)

Comment 15

2 years ago
Created attachment 8657243 [details] [diff] [review]
bug827140.patch
Attachment #8636715 - Attachment is obsolete: true
Attachment #8657243 - Flags: review?(jlong)
(Assignee)

Comment 16

2 years ago
Created attachment 8657259 [details] [diff] [review]
bug827140.patch
Attachment #8657243 - Attachment is obsolete: true
Attachment #8657243 - Flags: review?(jlong)
Attachment #8657259 - Flags: review?(jlong)
+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 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 :)
Attachment #8657259 - Flags: review?(jlong) → review+
(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?
(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.
(Assignee)

Comment 21

2 years ago
Created attachment 8672768 [details] [diff] [review]
bug827140.patch
Attachment #8657259 - Attachment is obsolete: true
Attachment #8672768 - Flags: review+
(Assignee)

Comment 22

2 years ago
(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
(Assignee)

Updated

2 years ago
Flags: needinfo?(jlong)
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.
Flags: needinfo?(jlong)
Keywords: checkin-needed

Comment 24

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/9860b9cd1653
Keywords: checkin-needed

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9860b9cd1653
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(Assignee)

Comment 26

2 years ago
Nice! Thanks :jlongster for your help.

Comment 27

2 years ago
[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.
Flags: needinfo?(saneyuki.s.snyk)
I verified this bug has been fixed. Thank you!
Status: RESOLVED → VERIFIED
Flags: needinfo?(saneyuki.s.snyk)
You need to log in before you can comment on or make changes to this bug.