New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mdouglass opened this Issue Apr 24, 2018 · 26 comments

Comments

Projects
None yet
@mdouglass
Contributor

mdouglass commented Apr 24, 2018

  • 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

> temp@1.0.0 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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 24, 2018

Member

Ping @addaleax ... any ideas?

Member

jasnell commented Apr 24, 2018

Ping @addaleax ... any ideas?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 24, 2018

Member

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

Member

jasnell commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Apr 24, 2018

Member

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

Member

apapirovski commented Apr 24, 2018

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

@tirthamazumdar

This comment has been minimized.

Show comment
Hide comment
@tirthamazumdar

tirthamazumdar Apr 24, 2018

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)

tirthamazumdar commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 24, 2018

Member

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

Member

jasnell commented Apr 24, 2018

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

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Apr 24, 2018

Member

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

Member

apapirovski commented Apr 24, 2018

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

@tirthamazumdar

This comment has been minimized.

Show comment
Hide comment
@tirthamazumdar

tirthamazumdar Apr 24, 2018

@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

tirthamazumdar commented Apr 24, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 24, 2018

Member

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.

Member

jasnell commented Apr 24, 2018

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.

@addaleax addaleax added the buffer label Apr 24, 2018

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 24, 2018

Member

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

Member

jasnell commented Apr 24, 2018

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

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Apr 24, 2018

Member

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

Member

apapirovski commented Apr 24, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 24, 2018

Member

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?

Member

addaleax commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 24, 2018

Member

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

Member

jasnell commented Apr 24, 2018

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

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 24, 2018

Member

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?

Member

bnoordhuis commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Apr 24, 2018

Member

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

Member

apapirovski commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 24, 2018

Member

@bnoordhuis I’m talking about this:

node/lib/internal/util.js

Lines 343 to 352 in ad5307f

getStructuredStack = runInNewContext(`(function() {
Error.prepareStackTrace = function(err, trace) {
err.stack = trace;
};
Error.stackTraceLimit = Infinity;
return function structuredStack() {
return new Error().stack;
};
})()`, {}, { filename: 'structured-stack' });

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.

Member

addaleax commented Apr 24, 2018

@bnoordhuis I’m talking about this:

node/lib/internal/util.js

Lines 343 to 352 in ad5307f

getStructuredStack = runInNewContext(`(function() {
Error.prepareStackTrace = function(err, trace) {
err.stack = trace;
};
Error.stackTraceLimit = Infinity;
return function structuredStack() {
return new Error().stack;
};
})()`, {}, { filename: 'structured-stack' });

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 comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Apr 24, 2018

Member

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.

Member

rvagg commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Apr 24, 2018

Member

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'
Member

apapirovski commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Apr 24, 2018

Member

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.

Member

apapirovski commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 24, 2018

Member

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 ☝️

Member

Trott commented Apr 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Apr 25, 2018

Member

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

Member

richardlau commented Apr 25, 2018

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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 25, 2018

Member

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.

Member

bnoordhuis commented Apr 25, 2018

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.

pimterry added a commit to resin-io-modules/balena-settings-client that referenced this issue Apr 25, 2018

Temporarily stop testing in latest Node due to a Node 10 bug
Tests fail due to nodejs/node#20258. We can
and should re-enable this once that's been released.

smhxx added a commit to smhxx/atom-ts-transpiler that referenced this issue Apr 26, 2018

chore(travis): temporarily pin Node.js version to 9
This should resolve the current build failures and can be reverted
once nodejs/node#20258 is resolved. See #40.
@jy95

This comment has been minimized.

Show comment
Hide comment
@jy95

jy95 Apr 29, 2018

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

jy95 commented Apr 29, 2018

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

isurusiri added a commit to isurusiri/node that referenced this issue Apr 30, 2018

util: fix isInsideNodeModules inside error
When isInsideNodeModules gets called while already processing
another stack trace, V8 will not call prepareStackTrace again.
This used to cause Node.js to just crash — fix it by checking
for expected return type of the stack (Array).

PR-URL: nodejs#20266
Fixes: nodejs#20258
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@yoav-zibin

This comment has been minimized.

Show comment
Hide comment
@yoav-zibin

yoav-zibin May 4, 2018

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

yoav-zibin commented May 4, 2018

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

This comment has been minimized.

Show comment
Hide comment
@hashseed

hashseed May 4, 2018

Member

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

Member

hashseed commented May 4, 2018

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

MylesBorins added a commit that referenced this issue May 4, 2018

util: fix isInsideNodeModules inside error
When isInsideNodeModules gets called while already processing
another stack trace, V8 will not call prepareStackTrace again.
This used to cause Node.js to just crash — fix it by checking
for expected return type of the stack (Array).

PR-URL: #20266
Fixes: #20258
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski May 5, 2018

Member

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

Member

apapirovski commented May 5, 2018

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

@mariusvw

This comment has been minimized.

Show comment
Hide comment
@mariusvw

mariusvw May 11, 2018

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

mariusvw commented May 11, 2018

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

neel015 referenced this issue in Microsoft/azure-pipelines-image-generation May 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment