util: fix isInsideNodeModules inside error#20266
util: fix isInsideNodeModules inside error#20266apapirovski wants to merge 1 commit intonodejs:masterfrom
Conversation
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).
nodejs-github-bot
added
the
util
| if (!/^\/|\\/.test(filename)) | ||
| continue; | ||
| return kNodeModulesRE.test(filename); | ||
| if (Array.isArray(stack)) { |
There was a problem hiding this comment.
Maybe just invert the conditional to make here?
There was a problem hiding this comment.
Yeah, I just wanted to keep the semantics exactly the same as before so return false; after everything, even if it's strictly-speaking unreachable from the for loop.
| if (!/^\/|\\/.test(filename)) | ||
| continue; | ||
| return kNodeModulesRE.test(filename); | ||
| if (Array.isArray(stack)) { |
There was a problem hiding this comment.
Is this sufficient to fix the original bug with ts-node? That was triggering at original line 360 with frame existing but getFileName being undefined, which is inside the for loop. That seems to imply to me that it would not be fixed by an Array.isArray check. I'm not sure how to grab this to confirm that though.
There was a problem hiding this comment.
Yeah, it's because if stack is not an array then it's a string so it iterates on individual characters which obviously don't have getFileName.
There was a problem hiding this comment.
It might be good to throw in an extra check for the existence of getFileName as a function, just to be extra safe.
There was a problem hiding this comment.
Hmmm... but we do want to minimize how much work we do here. There are two possibilities here:
-
We call our own
prepareStackTracewhich returns an Array with the correct data. -
We are already in an error handling mode and just get the default stack trace which is a string, not an array.
There's no real alternative here. We can't end up calling a user-provided prepareStackTrace or something similar. I did think about it before making this check.
|
node-test-commit-smartos re-run: https://ci.nodejs.org/job/node-test-commit-smartos/17152/ |
Trott
added
the
author ready
|
node-test-commit-aix re-run: https://ci.nodejs.org/job/node-test-commit-aix/14493/ |
|
Thanks everyone. Landed in 16aee38 |
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>
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>
7 participants
When
isInsideNodeModulesgets called while already processinganother stack trace, V8 will not call
prepareStackTraceagain.This used to cause Node.js to just crash — fix it by checking
for expected return type of the stack (
Array).Fixes: #20258
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes