node 10.0.0 with ts-node throws a TypeError in isInsideNodeModules when an error is thrown by user code

[mdouglass]

• Version: v10.0.0
• Platform: Darwin bigbird.local 17.5.0 Darwin Kernel Version 17.5.0: Mon Mar 5 22:24:32 PST 2018; root:xnu-4570.51.1~1/RELEASE_X86_64 x86_64

Attached sample project (which is a single ts file with just throw new Error()) when run via npm start fails with:

mdouglass$ npm start

> [email protected] start /Users/mdouglass/kixeye/km/server/temp
> ts-node index.ts

internal/util.js:360
    const filename = frame.getFileName();
                           ^

TypeError: frame.getFileName is not a function
    at isInsideNodeModules (internal/util.js:360:28)
    at showFlaggedDeprecation (buffer.js:149:8)
    at new Buffer (buffer.js:174:3)
    at Array.<anonymous> (/Users/mdouglass/kixeye/km/server/temp/node_modules/source-map-support/source-map-support.js:163:21)
    at /Users/mdouglass/kixeye/km/server/temp/node_modules/source-map-support/source-map-support.js:53:24
    at mapSourcePosition (/Users/mdouglass/kixeye/km/server/temp/node_modules/source-map-support/source-map-support.js:185:21)
    at wrapCallSite (/Users/mdouglass/kixeye/km/server/temp/node_modules/source-map-support/source-map-support.js:357:20)
    at /Users/mdouglass/kixeye/km/server/temp/node_modules/source-map-support/source-map-support.js:392:26
    at Array.map (<anonymous>)
    at Function.prepareStackTrace (/Users/mdouglass/kixeye/km/server/temp/node_modules/source-map-support/source-map-support.js:391:24)

repro-nodejs-10-throw.zip

[jasnell]

Ping @addaleax ... any ideas?

[jasnell]

This is possibly a bad interaction with make-error but I need to verify
I take that back... https://github.com/evanw/node-source-map-support/blob/master/source-map-support.js looks like a better candidate...

(heh... it would help if I actually read the entire stack track ... lol...)

[apapirovski]

I'm looking into this but anyone else is also welcome to dig in.

[tirthamazumdar]

I do have the same issue. Just now upgraded to 10.0.0 from version 8, and all my tests are failing.

internal/util.js:360
const filename = frame.getFileName();
^
TypeError: frame.getFileName is not a function
at isInsideNodeModules (internal/util.js:360:28)
at showFlaggedDeprecation (buffer.js:149:8)
at new Buffer (buffer.js:174:3)

[jasnell]

@tirthamazumdar ... are you also using source-map-support?

[apapirovski]

@jasnell It just comes with ts-node. source-map-support is definitely what's causing it but still not sure how to resolve.

[tirthamazumdar]

@jasnell yes I am also using source-map-support.. Just now compiled typescript into javascript and the running directly the javascript tests the problem goes away.
Looks like issue is with source-map-support

[jasnell]

Ok, @apapirovski @addaleax ... looks like the transpiler here is mucking around with the generation of the stack frames in a way the check in core did not anticipate. I think we should likely do in this case is have isInsideNodeModules() return false if it cannot reliably process the stack. That would cause the Buffer deprecation warning to emit in this case, but that's not really a bad thing.

[jasnell]

Attempting to put together a standalone repo test case now but not having much luck yet.

[apapirovski]

@jasnell Here's the simplest possible reproduction:

Error.prepareStackTrace = (err, trace) => new Buffer();

new Error().stack;

The error happens because V8 won't call prepareStackTrace if it's already preparing a stack trace (recursive call). I don't think we can fix so, as you said, we'll just have to skip the check and emit the warning in those cases.

PR coming up.

[addaleax]

Ouch. Yes, that makes sense – thanks for figuring this out so quick. I am surprised though, it might be nice if V8 supported this – after all, the other Error object does come from a difference VM Context, so one wouldn’t expect that kind of interaction … @nodejs/v8?

[jasnell]

I had seen the Buffer use there but hadn't realized it was a recursive check. Good catch.

[bnoordhuis]

the other Error object does come from a difference VM Context

Not sure I follow. If it's about @apapirovski's test case, what other Error object and VM context?

[apapirovski]

Not sure I follow. If it's about @apapirovski's test case, what other Error object and VM context?

isInsideNodeModules generates a stack-trace yielding function in a VM to get the proper stack trace. See lib/internal/util.js around line ~340. It also uses prepareStackTrace which in V8 is guarded against being called recursively

[addaleax]

@bnoordhuis I’m talking about this:

https://github.com/nodejs/node/blob/ad5307f1b4bc2d6824207717a2964c917aff1cb9/lib/internal/util.js#L343-L352

It’s a different context, and while I get the intention on V8’s side, this does seem surprising; it’s not like it’s recusing into the same prepareStackTrace function here as the outer one.

[rvagg]

this requires that an error be thrown to show up at all right? so there's no way we could have picked this up by having anything extra in citgm? breaking TS support is becoming a bigger deal as its adoption spreads.

[apapirovski]

I think maybe having https://github.com/evanw/node-source-map-support in there would've caught it? But I haven't checked the test suite.

Edit: Yep, would've caught it:

```
28 passing (1s)
1 failing

1) should allow for runtime inline source maps:

  AssertionError [ERR_ASSERTION]: 'TypeError: frame.getFileName is not a function' == 'Error: this is the error'

[apapirovski]

Thinking about it more, it's probably not the worst module to add. There are a few less common APIs exercised in their module & test suite.

[Trott]

I think maybe having https://github.com/evanw/node-source-map-support in there would've caught it? But I haven't checked the test suite.

Edit: Yep, would've caught it:

@nodejs/citgm ☝️

[richardlau]

I think maybe having https://github.com/evanw/node-source-map-support in there would've caught it? But I haven't checked the test suite.

Edit: Yep, would've caught it:

@nodejs/citgm ☝️

Pull requests welcome over at https://github.com/nodejs/citgm. Please make sure to document in the PR the fulfilled hard and soft requirements met by the module in question as per: https://github.com/nodejs/citgm/blob/master/CONTRIBUTING.md#submitting-a-module-to-citgm

[bnoordhuis]

It’s a different context, and while I get the intention on V8’s side, this does seem surprising; it’s not like it’s recusing into the same prepareStackTrace function here as the outer one.

Okay, I get it now. Wouldn't be terribly hard to fix in V8 at the cost of some additional complexity (tracking the entered contexts.)

That said, the goal of isInsideNodeModules() is to find the first non-core stack frame and there are arguably better ways of doing that than using a custom Error.prepareStackTrace.

Strawman: capture the stack trace with v8::StackTrace::CurrentStackTrace() and find the first frame with a script id that isn't of a built-in module.

We don't currently record our own script ids but that's relatively straightforward to add.

[jy95]

I found related to source-map-support in my broken build : https://travis-ci.org/jy95/mediaScan/jobs/372789737#L517

[yoav-zibin]

What's the solution?

$ node -v
v10.0.0
$ npm -v
6.0.0

TypeError: frame.getFileName is not a function
at isInsideNodeModules (internal/util.js:360:28)
at showFlaggedDeprecation (buffer.js:149:8)
at new Buffer (buffer.js:174:3)
at Array. (/Users/yzibin/GitHub/NewGamePortal/node_modules/source-map-support/source-map-support.js:149:21)
at /Users/yzibin/GitHub/NewGamePortal/node_modules/source-map-support/source-map-support.js:53:24
at mapSourcePosition (/Users/yzibin/GitHub/NewGamePortal/node_modules/source-map-support/source-map-support.js:171:21)
at wrapCallSite (/Users/yzibin/GitHub/NewGamePortal/node_modules/source-map-support/source-map-support.js:343:2

[hashseed]

V8 only has a single flag for checking whether prepareStackTrace is recursing. Are you saying having the flag per context or stored on the prepareStackTrace function itself would solve this? @schuay

[apapirovski]

@yoav-zibin 10.1.0 should come out soon and have a fix for it.

[mariusvw]

Just upgraded, can confirm that after upgrading to 10.1.0 everything works alright on our machines.