1616196 - Wrong frame name for anonymous function

Status: NEW

(DevTools :: Debugger, defect, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

STR:

1. Load https://fishy-desert.glitch.me/
2. Create BP on line 8
3. Click the Click Me! button on the page, the debugger should pause
4. Check out the Callstack side panel it shows func1 as the latest (youngest) frame => BUG

AR: The latest frame is labeled as func1

ER: The latest frame name should be anonymous since the callback (passed to then) doesn't have a name)

Honza

Comment 1

[Logan Smyth [:loganfsmyth]]

Attachment: Screen Shot 2020-02-19 at 6.31.10 PM.png

I'd call this expected behavior because it matches with the synchronous case. For example if you run this snippet:

function func1() {
  (() => {
    debugger;
  })();
}

func1();

you end up with the attached call stack.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

True, I see the consistency.

Note that Chrome is showing a callstack (attached) with anonymous functions

Honza

Comment 3

[Logan Smyth [:loganfsmyth]]

I have no complaints if we want to change this for chrome parity, but if we do I'd want to change it for the sync case as well.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I don't think it's a blocker for async captured stacks.

Harald, WDYT?

Honza

Comment 5

[:Harald Kirschner :digitarald]

Agreed, we'll see if more people hit this.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

Another case where I am running in this problem and it's more visible

STR:

1. Load https://relieved-cesium.glitch.me/
2. Open client.js and create BP on line 19
3. Click the button on the page to hit the BP
4. Check out the callstack

AR: it looks like the waitForTime is executed 3 times, but it's just one time
ER: the Callstack should show anonymous for line 19 and 19

Perhaps we should yet reconsider the priority.

Honza

Comment 7

[:Harald Kirschner :digitarald]

Logan, without too much digging, might bug 1627599 have the same root cause?

Comment 8

[Logan Smyth [:loganfsmyth]]

Yep all the same issue. I'll close the dup and elaborate on what we're seeing and why so we can decide what we want to do.

There's two related views of function name. The one shown by SpiderMonkey in error traces (and I think other places), and the function name shown by the debugger itself. From SpiderMonkey's standpoint,

function func1() {
  (() => {
    console.log(new Error().stack);
  })();
}
func1();

is a function named "funct1" with an "guessed name" that is "func1/<" which isn't visible to the JS code itself except via the stack trace during an error. If you run that snippet in the console you'll get

func1/<@debugger eval code:3:17
func1@debugger eval code:4:5
@debugger eval code:7:1

where you can see the name of the function is not the same as the parent function, so SpiderMonkey tries to give things names while not just reusing the name of the wrapper function.

Presently, DevTools explicitly strips off the /< part presumably because it was deemed too confusing for users or something, but I don't really know. https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/devtools/client/debugger/packages/devtools-reps/src/reps/function.js#170

V8 does not have this "guessed name" behavior, and they just leave the function as fully anonymous, so the snippet above produces

Error
    at <anonymous>:3:17
    at func1 (<anonymous>:4:5)
    at <anonymous>:7:1

So the question here has two parts:

1. Should DevTools shown this as an anonymous function, or pass through "func1/<` as a kind of weird implicit name?
2. Should SpiderMonkey itself drop this "guessed name" from its stack traces? Do we care if the string trace from SpiderMonkey doesn't match the DevTools UI?

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Since we're already relying on a regex to clean the function name, I guess we could check the existence of /< and in such case display anonymous.
Jason, do you have thoughts on that? I know you studied function naming :)

Comment 11

[Logan Smyth [:loganfsmyth]]

Oh I should have clarified that part, I think we can address it more directly by dropping all the usage of grip.displayName in https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/devtools/client/debugger/packages/devtools-reps/src/reps/function.js#146-157. We already have grip.name for the actual name of the function, it's just that we prefer the guessed grip.displayName when I don't think we should.

Comment 12

[:Harald Kirschner :digitarald]

Sounds like a good topic for the team meeting.

It seems like we are losing some fidelity by just calling functions anonymous; but get closer to what Chrome does. I'd like to understand why we removed it in the first place though.

Comment 13

[Jason Laster [:jlast]]

That makes sense Nicolas.

I think we might want to re-evaluate how we come up with names for anonymous and whether we want to.

For example, this case seems legit { a: () => {} }

If we're not using the crazy syntax, I know jorendorff would be happy to remove it.

Comment 14

[Logan Smyth [:loganfsmyth]]

From at least a casual inspection, all of the /< stuff was added before ES6 was specified and added more cases where function names are inferred, so for example

var obj = { a: () => {} };

obj.a.name; // "a"

var foo = () => {};
foo.name; // "foo"

so the number of cases where a function is truly anonymous is also not as large as it was when this logic was added. These days the main cases I'd expect a function to be truly anonymous is when
a) It is assigned to an existing object, e.g. o.prop = function(){ debugger; };
b) It is declared and passed as a callback, e.g. runCallback(() => { debugger; })

While those are likely still certainly happening in real codebases, it's at least not as much as before, and we still have the line/column locations to know the exact function location.

Comment 15

[:Harald Kirschner :digitarald]

We decided to switch to anonymous function names, as comment 10 describes. This assumes that those functions are mostly displayed in stacks where the parent function is visible; so the /< notation doesn't add more context, but rather confusion as most devs don't understand it.

Comment 16

[Jason Laster [:jlast]]

Here's a quick sketch of what the code fix could look like with a unit test change

https://gist.github.com/dcfaa8f18290f9e2296575418fa4cd4d

I didn't verify that the functionProperty is what we want, but it seems right...

Here's a good test case -- https://anonymous-func.glitch.me/ where we should pause in an object property anonymous function and a callback function. In the obj property we want to show the property as the display name, in the callback we want to show anonymous.

Comment 17

[Stephanie Cunnane [:scunnane]]

Attachment: call_stack_not_showing_anonymous_function.png

Comment 18

[Stephanie Cunnane [:scunnane]]

As another data point, I had a real "sanity check" moment this week because of this issue.

Looking at the attached screenshot, it made no sense to me why the code was calling the parent function (_setInitialFocus) twice. But after closer examination, I realized that, with execution paused inside the anonymous function on line 2178, the call stack didn't reflect the fact that I was inside the anonymous function. Instead it incorrectly said I was in the parent function.