585956 - Implement console.trace() in web console

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

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

We need the ability to get access to JS stack traces from the console. It's currently a bit of a pain to get these out of the browser.

see, http://www.mozilla.org/scriptable/javascript-stack-dumper.html

Not sure how we should do this or expose this to a developer. We could create a new console function (e.g., console.dumpStack()) that we could use inside content (or chrome) code to drop a stack trace into the console at a specific point in code.

Comment 1

[Wesley W. Garland]

If we had a solution for bug 639800 (stack pretty printer) that took an error-like object as it's parameter, developers could do this:

console.printStack(new Error())

and have the console print the stack as it was right on that line of code.  I think that's the only practical way to get that information out of JSAPI without turning on a debugger, setting a trap, and walking the stack.

Comment 2

[Kevin Dangoor]

This is connected to bug 644596 (add missing methods to the console object). Firebug and WebKit implement stack trace display as console.trace().

Comment 3

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

good catch. Let's rename this.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: wip 1

First WIP patch.

The basic functionality is there already:

- you get a stack trace in the web console when you call console.trace().
- you see filename, function name and line number.
- you can click to inspect the stack trace, you see each frame in the Property Panel. This allows you to see the file name, function name and line number of each frame in the stack.

What's missing: a mochitest and small fixes in some other mochitest that fails now.

I'd like feedback on this if it's fine.

General notes:

- as discussed with Kevin we could do in the future a nicer stack viewer than the Property Panel. As I understand, that's beyond the purpose of this bug. There would be a lot of UI churn.

- Firebug gets the trace differently. It uses the debugger service and more complicated approaches. The stack trace it gets is nicer - it includes the function arguments and it can determine event handlers as well. I think that's something we should look into a more advanced "phase 2" implementation of console.trace().

- I use Components.stack which is quite awesome. See:

https://developer.mozilla.org/en/Components.stack

and https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIStackFrame

... now, I was rather dismayed to see that sourceLine doesn't hold the source code, it's always null. I am tempted to take a dive into the implementation:

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcstack.cpp#310

and just make it work. It would be very useful for us to show authors the source line, not just the number. Shall I do this? Or shall we make this in a separate bug? Or it's not high priority?

- If I take a dive into the C++ side of stack frames, I'd like to add the function arguments as well. Thoughts on this?

Looking forward to your feedback. Thanks!

Comment 5

[David Dahl :ddahl]

Comment on attachment 524449 [details] [diff] [review]
wip 1

>+   * Build the stacktrace array for the console.trace() call.
>+   *
>+   * @return array
>+   *         Each element is a stack frame that holds the following properties:
>+   *         filename, lineNumber, functionName and language.
>+   **/
>+  getStackTrace: function CA_getStackTrace() {
>+    let stack = [];
>+    let frame = Components.stack.caller;

Does this get any part of the chrome privileged stack?

>+
>+        clipboardText = clipboardText.trimRight();
>+        break;
>+
>+      default:
>+        throw Components.Exception("Unknown Console API log level: " + aLevel,
>+                                   Cr.NS_ERROR_INVALID_ARG,
>+                                   Components.stack.caller);
I don't think you should throw here - perhaps just Cu.reportError?

Looks good

Comment 6

[Mihai Sucan [:msucan]]

(In reply to comment #5)
> Comment on attachment 524449 [details] [diff] [review]
> wip 1
> 
> >+   * Build the stacktrace array for the console.trace() call.
> >+   *
> >+   * @return array
> >+   *         Each element is a stack frame that holds the following properties:
> >+   *         filename, lineNumber, functionName and language.
> >+   **/
> >+  getStackTrace: function CA_getStackTrace() {
> >+    let stack = [];
> >+    let frame = Components.stack.caller;
> 
> Does this get any part of the chrome privileged stack?

Yes, it does. This is why I skip the first frame. The stack even includes some native c++ frames.

I know the worry is we might include chrome stuff in the stack, but based on testing the stack is pretty clean, because we have the page JS stack followed immediately by one or two native c++ frame, then the stack frames of ConsoleAPI (which is short and sweet so-to-speak).


> >+        clipboardText = clipboardText.trimRight();
> >+        break;
> >+
> >+      default:
> >+        throw Components.Exception("Unknown Console API log level: " + aLevel,
> >+                                   Cr.NS_ERROR_INVALID_ARG,
> >+                                   Components.stack.caller);
> I don't think you should throw here - perhaps just Cu.reportError?

Will change that then.

> Looks good

Thanks for looking into it!

When you have time, I'd appreciate answers to the questions I raised in comment 4. Thanks!

Comment 7

[Wesley W. Garland]

> The stack even includes some native c++ frames

Is this still true in Firefox 4? I was under the impression that we might lose these as a result of changing everything over to the fast-native calling convention.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> > The stack even includes some native c++ frames
> 
> Is this still true in Firefox 4? I was under the impression that we might lose
> these as a result of changing everything over to the fast-native calling
> convention.

This is Firefox 4 we are working with. The native frames don't have proper location info, they just show as "native frames", having .languageName CPLUSPLUS. I don't know about the fast-native calling convention stuff. Pointers?

Beyond that, the stack does definitely *not* include all the native frames, since that would mean showing a lot more info than it does now. I am surprised only a couple or so of native frames show. In my opinion it should only display JS stuff and that's all.

Anyway, this is really beyond the purpose of the bug we are trying to fix. As long as we display a usable stack of frames when devs call console.trace(), we are fine. ;)

Comment 9

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This is the proposed patch, with some fixes, nicer output in the Web Console (new strings!), and tests for console.trace().

Re-asking for feedback+ since you didn't get to see the complete patch - yesterday was only a WIP, to ask if the approach is fine for you. Then we shall move it to review. Do we need to ask for l10n review as well?

Thanks for looking into the patch!

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 524680 [details] [diff] [review]
proposed patch

Thanks for the f+! Asking for review from Robert.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased to the latest devtools branch.

Comment 12

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

Comment on attachment 524977 [details] [diff] [review]
rebased patch

make sure you add trace() to /dom/tests/mochitest/general/test_consoleAPI.html. I missed it when I added the debug function to consoleAPI in bug 616742.

+  getStackTrace: function CA_getStackTrace() {

I think we're going to need a security review on this. You're calling some heavy stuff from content with this method.

Deferring ultimate review on this to Jonas, but functionally, it looks correct. 

Is the stop condition sufficient in your while loop? It likely is (it'll get to the top eventually) but you might be able to break out of it earlier.

"trace" case in HUDservice looks good.

I think a run through try is in order.

Comment 13

[Mihai Sucan [:msucan]]

(In reply to comment #12)
> Comment on attachment 524977 [details] [diff] [review]
> rebased patch
> 
> make sure you add trace() to /dom/tests/mochitest/general/test_consoleAPI.html.
> I missed it when I added the debug function to consoleAPI in bug 616742.

Will do. Thanks!


> +  getStackTrace: function CA_getStackTrace() {
> 
> I think we're going to need a security review on this. You're calling some
> heavy stuff from content with this method.
> 
> Deferring ultimate review on this to Jonas, but functionally, it looks correct. 
> 
> Is the stop condition sufficient in your while loop? It likely is (it'll get to
> the top eventually) but you might be able to break out of it earlier.
> 
> "trace" case in HUDservice looks good.
> 
> I think a run through try is in order.

Will push to try server the updated patch.

Comment 14

[Mihai Sucan [:msucan]]

Attachment: [checked-in][in-devtools] updated patch

Rebased patch on top of the latest devtools repo and updated dom/tests/mochitest/general/test_consoleAPI.html as requested.

Thanks Rob for looking into the patch. I didn't know about that console API test file.

Will push this patch to the try server.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 525106 [details] [diff] [review]
[checked-in][in-devtools] updated patch

r=me on the glue code only (which is the part I suspect that you wanted me to review?)

Comment 16

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

(In reply to comment #15)
> Comment on attachment 525106 [details] [diff] [review]
> updated patch
> 
> r=me on the glue code only (which is the part I suspect that you wanted me to
> review?)

yep, thanks!

Comment 17

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

Comment on attachment 525106 [details] [diff] [review]
[checked-in][in-devtools] updated patch

r+ on the rest.

Comment 18

[Mihai Sucan [:msucan]]

Try server results:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=fe21458ee2b0

They are looking good until now, but they are still running (?).

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 525106 [details] [diff] [review]
[checked-in][in-devtools] updated patch

http://hg.mozilla.org/projects/devtools/rev/66021ffa6c09

Comment 20

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

Comment on attachment 525106 [details] [diff] [review]
[checked-in][in-devtools] updated patch

http://hg.mozilla.org/mozilla-central/rev/66021ffa6c09

Comment 21

[AndreiD[QA]]

Marking this as verified.

Comment 22

[Jean-Yves Perrier [:teoli]]

:sheppy, do you think we know more doc about this than already in 
https://developer.mozilla.org/en/Using_the_Web_Console
or should we set the keyword to dev-doc-complete ?