1184172 - Show the stack in console for exception

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Alexandre Poirot [:ochameau]]

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.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Is this a duplicate of bug 1141222 (or the other way around)?

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Comment 3

[Alexandre Poirot [:ochameau]]

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.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

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)

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(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.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Fixed the layout issue and added a test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecda9ff7aeb6

Comment 7

[Alexandre Poirot [:ochameau]]

(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.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6becb208bf83

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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?

Comment 10

[Alexandre Poirot [:ochameau]]

(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

[Ed Lee :Mardak]

Attachment: 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 12

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 13

[Alexandre Poirot [:ochameau]]

(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.

Comment 14

[Alexandre Poirot [:ochameau]]

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.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

See comment 14.

Comment 16

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 17

[Pulsebot]

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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e556ecd514e0

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

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!