Show the stack in console for exception

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks 1 bug)

unspecified
Firefox 43
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed, relnote-firefox 43+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

4 years ago
Now that bug 814497 is about to be fixed, we need to tweak the actor and the UI to show the stack.
We currently support showing stack of console API call but exceptions go throw another codepath that doesn't consider stacks.
Assignee

Updated

4 years ago
Assignee: nobody → poirot.alex
Is this a duplicate of bug 1141222 (or the other way around)?
Assignee

Comment 2

4 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Assignee

Comment 3

4 years ago
Comment on attachment 8636567 [details] [diff] [review]
patch v1

Panos, The platform is now ready to pass the Stack object!
But make it work on the UI part isn't that easy due to the class hierarchy used for console messages. What do you think about moving stack display to the toplevel class? The display is messed up for some reasons...

You can play with this patch by going to this example:
  https://bugzilla.mozilla.org/attachment.cgi?id=684492
and opening the console.
Attachment #8636567 - Flags: feedback?(past)
Comment on attachment 8636567 [details] [diff] [review]
patch v1

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

Didn't look too closely into the style/layout issues, but I like the approach.

::: toolkit/devtools/server/actors/webconsole.js
@@ +1266,5 @@
> +    // Convert stack objects to the JSON attributes expected by client code
> +    if (aPageError.stack) {
> +      stack = [];
> +      let s = aPageError.stack;
> +      while (s) {

Maybe an empty string isn't allowed by the platform code, but |while (s !== null)| feels safer (or != if it can be undefined).

@@ +1270,5 @@
> +      while (s) {
> +        stack.push({
> +          filename: s.source,
> +          lineNumber: s.line,
> +          functionName: s.functionDisplayName

I hope functionDisplayName does the right thing in the presence of fn.displayName or statically inferred function names.

Why is the column number missing? We do seem to get the column in the error itself. Doesn't the platform generate column numbers everywhere by now?

And perhaps we should include "language: 2" for consistency as well? Here is a stack from a console API call:

    "stacktrace": [
      {
        "columnNumber": 5,
        "filename": "Scratchpad/1",
        "functionName": "k",
        "language": 2,
        "lineNumber": 3
      },
      {
        "columnNumber": 3,
        "filename": "Scratchpad/1",
        "functionName": "f",
        "language": 2,
        "lineNumber": 5
      },
      {
        "columnNumber": 1,
        "filename": "Scratchpad/1",
        "functionName": "",
        "language": 2,
        "lineNumber": 7
      }
    ],

@@ +1294,5 @@
>        exception: !!(aPageError.flags & aPageError.exceptionFlag),
>        strict: !!(aPageError.flags & aPageError.strictFlag),
>        info: !!(aPageError.flags & aPageError.infoFlag),
>        private: aPageError.isFromPrivateWindow,
> +      stackframe: stack

s/stackframe/stacktrace/ (for clarity and consistency with console API packets)
Attachment #8636567 - Flags: feedback?(past) → feedback+
Status: NEW → ASSIGNED
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> Is this a duplicate of bug 1141222 (or the other way around)?

I don't think it's a duplicate, as this patch doesn't seem to fix that bug, but I think the required changes should now be minimal.

A probably related issue, is that the error from the example page mentioned in comment 3 doesn't carry a stack in the Browser Console, unlike the Web Console.
Assignee

Comment 6

4 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Fixed the layout issue and added a test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecda9ff7aeb6
Attachment #8636567 - Attachment is obsolete: true
Assignee

Comment 7

4 years ago
(In reply to Panos Astithas [:past] from comment #4)
> Comment on attachment 8636567 [details] [diff] [review]
> ::: toolkit/devtools/server/actors/webconsole.js
> @@ +1266,5 @@
> > +    // Convert stack objects to the JSON attributes expected by client code
> > +    if (aPageError.stack) {
> > +      stack = [];
> > +      let s = aPageError.stack;
> > +      while (s) {
> 
> Maybe an empty string isn't allowed by the platform code, but |while (s !==
> null)| feels safer (or != if it can be undefined).

Done.

> 
> @@ +1270,5 @@
> > +      while (s) {
> > +        stack.push({
> > +          filename: s.source,
> > +          lineNumber: s.line,
> > +          functionName: s.functionDisplayName
> 
> I hope functionDisplayName does the right thing in the presence of
> fn.displayName or statically inferred function names.

Looks like it, see the test.

> 
> Why is the column number missing? We do seem to get the column in the error
> itself. Doesn't the platform generate column numbers everywhere by now?

I just forgot about the column, but the platform does pass it.

> 
> And perhaps we should include "language: 2" for consistency as well? Here is
> a stack from a console API call:

Hum. This doesn't seem to be used anywhere expect in test where we hardcode a "2" which doesn't mean much. I would be up to not sending it here. I would even remove this if we really don't use it at all.

> @@ +1294,5 @@
> >        exception: !!(aPageError.flags & aPageError.exceptionFlag),
> >        strict: !!(aPageError.flags & aPageError.strictFlag),
> >        info: !!(aPageError.flags & aPageError.infoFlag),
> >        private: aPageError.isFromPrivateWindow,
> > +      stackframe: stack
> 
> s/stackframe/stacktrace/ (for clarity and consistency with console API
> packets)

Done.
Assignee

Updated

4 years ago
Attachment #8652371 - Flags: review?(past)
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Created attachment 8652371 [details] [diff] [review]
> patch v3
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6becb208bf83

Is there something special I need to do to get stack traces in the console? What is the feature supposed to look like?
Assignee

Comment 10

4 years ago
(In reply to Jeff Griffiths (:canuckistani) from comment #9)
> (In reply to Alexandre Poirot [:ochameau] from comment #8)
> > Created attachment 8652371 [details] [diff] [review]
> > patch v3
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6becb208bf83
> 
> Is there something special I need to do to get stack traces in the console?

No, but it can easily fail to get the stack given the number of different codepath we have to handle exception at platform level :s

I do support the original bug report in bug 814497. So please test it against attachment 684492 [details]. Report your usecase if the stack doesn't appear.
I'll fix that in another bug.

> What is the feature supposed to look like?

Otherwise, it looks a bit like console.trace, except that you see the exception message by default and you have a triangle icon to toggle in order to see the full trace.

Comment 11

4 years ago
Posted image v3 screenshot
(In reply to Alexandre Poirot [:ochameau] from comment #10)
> please test it against attachment 684492 [details]
Thanks. That triggers the triangle to expand the stack for me.

However, it doesn't trigger from the console repl. See attached.
Comment on attachment 8652371 [details] [diff] [review]
patch v3

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

The twisty is slightly off (needs some padding-top or something), but we can fix that in a followup if you prefer.
Attachment #8652371 - Flags: review?(past) → review+
Assignee

Comment 13

4 years ago
(In reply to Panos Astithas [:past] from comment #12)
> Comment on attachment 8652371 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8652371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The twisty is slightly off (needs some padding-top or something), but we can
> fix that in a followup if you prefer.

Yes, that would be great if I can land it as I'll be off for a week.
But fx-team is closed for now.
Assignee

Updated

4 years ago
Blocks: 1199405
Assignee

Updated

4 years ago
Blocks: 1199406
Assignee

Comment 14

4 years ago
Jeff, please tell me if that works or not. If not it could be the same case than comment 11 (opened bug 1199405 for that).

Also if you see any design/css polish, please comment in bug 1199406.
See comment 14.
Flags: needinfo?(jgriffiths)
(In reply to Alexandre Poirot [:ochameau] from comment #14)
> Jeff, please tell me if that works or not. If not it could be the same case
> than comment 11 (opened bug 1199405 for that).
> 
> Also if you see any design/css polish, please comment in bug 1199406.

I do see it now using the test page, looks fine. tbqh I might have been running the wrong try build, sorry.

I do think the visual design needs work, the expandos are quite subtle, will comment over in the other bug.
Flags: needinfo?(jgriffiths)
https://hg.mozilla.org/mozilla-central/rev/e556ecd514e0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Release Note Request (optional, but appreciated)
[Why is this notable]: suggested by dev tools team
[Suggested wording]: Show the stack in console for exceptions in Dev Tools Inspector
[Links (documentation, blog post, etc)]:

If you have improvements for the release note wording, please comment and needinfo me. Thanks!

Updated

4 years ago
See Also: → 125612

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.