Stack traces broken for static methods with --enable-source-maps

[mika-fischer]

Version

22.9.0

Platform

Microsoft Windows NT 10.0.22631.0 x64

Subsystem

No response

What steps will reproduce the bug?

With

class JSClass {
  static async foo() {
    throw new Error("JSClass");
  }
}

the stack trace without --enable-source-maps is

Error: JSClass
    at JSClass.foo (file:///C:/Users/mfischer/src/videmo/test/node-bug-enable-source-maps/index.mjs:3:15)

but with --enable-source-maps it becomes:

Error: JSClass
    at Function.foo (C:\Users\mfischer\src\videmo\test\node-bug-enable-source-maps\index.mts:3:11)

The class name is lost somewhere.

I also noticed that the class name is missing even without using --enable-source-maps when using prototype based inheritance instead of classes and also for napi classes created with napi_define_class (not included in repro below).

See detailed repro at https://github.com/mika-fischer/node-bug-enable-source-maps

So maybe the issue is not really with enable-source-map-support but that the standard CallSite serialization accesses some magic property that only works for real JS classes and is missing when using prototype based inheritance or napi_define_class. And enable-source-maps just uses the public accessors of CallSite, which don't seem to expose/use this property...

In any case I'm interested in both:

• --enable-source-map should retain the class names
• Static methods of classes defined with napi_define_class (and preferable also "classes" defined using protoype inheritance) should have the class name (with or without --enable-source-maps)

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

the stack trace should be

Error: JSClass
    at JSClass.foo (C:\Users\mfischer\src\videmo\test\node-bug-enable-source-maps\index.mts:3:11)

What do you see instead?

the stack trace is

Error: JSClass
    at Function.foo (C:\Users\mfischer\src\videmo\test\node-bug-enable-source-maps\index.mts:3:11)

Additional information

No response

[avivkeller]

I can't reproduce:

class JSClass {
    static async foo() {
        throw new Error("JSClass");
    }
}

JSClass.foo();

$ node --enable-source-maps test.js
...
Error: JSClass
    at JSClass.foo (/test.js:3:15)
...

$ node test.js
...
Error: JSClass
    at JSClass.foo (/test.js:3:15)
...

---

$ npx esbuild test.js --outfile=build.js --sourcemap --minify

$ node --enable-source-maps build.js
...
Error: JSClass
    at JSClass.foo (/test.js:3:15)
...

[avivkeller]

I've closed this, as it's not reproducible, but I'm happy to take another look if thats not the case.

Verify that your sourcemap is not faulty.

[mika-fischer]

Did you try my repo @redyetidev ?

I think there are several shortcuts in the sourcemap implementation where it uses the normal toString() implementation if the sourcemap is not present or trivial...

[mika-fischer]

@redyetidev I figured it out.

There are two issues:

1. V8 has special handling in SerializeJSStackFrame for receivers that are JSClassConstructors. That's why it doesn't show the correct name for prototype inheritance and napi classes. If the special case is widened to all JSFunctions everything works as expected (with the standard Error.prepareStackTrace implementation, that uses SerializeJSStackFrame). See mika-fischer@9802b28

2. The issue with --enable-source-maps is that the custom formatter obviously does not use the standard stack frame serialization but uses .getTypeName() etc. to format the stack frame itself. Unfortunately, the special handling mentioned above is not in GetTypeName but only in SerializeJSStackFrame, so that one cannot get the correct name via this interface... Moving the special handling from SerializeJSStackFrame to GetTypeName fixes the remaining issue with --enable-source-maps. See mika-fischer@3c30538 (updated: bed2637)

With these two patches I get:

❯ C:\Users\mfischer\src\libs\node\out\Release\node.exe .\index.mjs                     
Version: v23.0.0-pre
execArgv: []
Error: JSClass
    at JSClass.foo (file:///C:/Users/mfischer/src/videmo/test/node-bug-enable-source-maps/index.mjs:3:15)
    ...
Error: JSProto
    at JSProto.foo (file:///C:/Users/mfischer/src/videmo/test/node-bug-enable-source-maps/index.mjs:12:19)
    ...

❯ C:\Users\mfischer\src\libs\node\out\Release\node.exe --enable-source-maps .\index.mjs
Version: v23.0.0-pre
execArgv: ["--enable-source-maps"]
Error: JSClass
    at JSClass.foo (C:\Users\mfischer\src\videmo\test\node-bug-enable-source-maps\index.mts:3:11)
    ...
Error: JSProto
    at JSProto.foo (C:\Users\mfischer\src\videmo\test\node-bug-enable-source-maps\index.mts:13:13)
    ...

instead of

❯ node .\index.mjs                     
Version: v22.9.0
execArgv: []
Error: JSClass
    at JSClass.foo (file:///C:/Users/mfischer/src/videmo/test/node-bug-enable-source-maps/index.mjs:3:15)
    ...
Error: JSProto
    at Function.foo (file:///C:/Users/mfischer/src/videmo/test/node-bug-enable-source-maps/index.mjs:12:19)
    ...

❯ node --enable-source-maps .\index.mjs
Version: v22.9.0
execArgv: ["--enable-source-maps"]
Error: JSClass
    at Function.foo (C:\Users\mfischer\src\videmo\test\node-bug-enable-source-maps\index.mts:3:11)
    ...
Error: JSProto
    at Function.foo (C:\Users\mfischer\src\videmo\test\node-bug-enable-source-maps\index.mts:13:13)
    ...

What do you think?

Is this something that node is willing to patch to fix this? Or does this need to go through V8 (I never contributed there before...)?

[avivkeller]

CC @nodejs/v8

[joyeecheung]

Or does this need to go through V8 (I never contributed there before...)?

Yes, in principle patches to deps/v8 should land in upstream first before being backported to Node.js. See https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-V8.md#unfixed-upstream-bugs

[mika-fischer]

Upstream issue: https://issues.chromium.org/issues/371019881

[avivkeller]

Hey, I've closed this as it's now a V8 patch, and is being tracked at https://chromium-review.googlesource.com/c/v8/v8/+/5907815. In other words, IMO it's out of our hands. Feel free to reopen if you disagree.

[avivkeller]

Actually, we still need to backport the issue to the project, so this should remain open haha sorry