lib,src: isInsideNodeModules should test on the first non-internal frame

PR-URL: #60991
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Author: legendecas

benchmark/internal/util_isinsidenodemodules.js

@@ -4,14 +4,13 @@ const assert = require('assert');
 
 constbench=common.createBenchmark(main,{
 n: [1e6],
-stackLimit: [100],
 stackCount: [99,101],
 method: ['isInsideNodeModules','noop'],
 },{
 flags: ['--expose-internals','--disable-warning=internal/test/binding'],
 });
 
-functionmain({ n,stackLimit,stackCount, method }){
+functionmain({ n, stackCount, method }){
 const{ internalBinding }=require('internal/test/binding');
 const{ isInsideNodeModules }=internalBinding('util');
 
@@ -30,7 +29,7 @@ function main({ n, stackLimit, stackCount, method }) {
 
 bench.start();
 for(leti=0;i<n;i++){
-isInsideNodeModules(stackLimit,true);
+isInsideNodeModules();
 }
 bench.end(n);
 };

lib/buffer.js

@@ -178,15 +178,14 @@ function showFlaggedDeprecation() {
 if(bufferWarningAlreadyEmitted||
 ++nodeModulesCheckCounter>10000||
 (!require('internal/options').getOptionValue('--pending-deprecation')&&
-isInsideNodeModules(100,true))){
+isInsideNodeModules(3))){
 // We don't emit a warning, because we either:
 // - Already did so, or
 // - Already checked too many times whether a call is coming
 // from node_modules and want to stop slowing down things, or
 // - We aren't running with `--pending-deprecation` enabled,
 // and the code is inside `node_modules`.
-// - We found node_modules in up to the topmost 100 frames, or
-// there are more than 100 frames and we don't want to search anymore.
+// - If the topmost non-internal frame is not inside `node_modules`.
 return;
 }
 

lib/internal/modules/cjs/loader.js

@@ -1537,9 +1537,8 @@ function loadESMFromCJS(mod, filename, format, source) {
 }elseif(mod[kIsCachedByESMLoader]){
 // It comes from the require() built for `import cjs` and doesn't have a parent recorded
 // in the CJS module instance. Inspect the stack trace to see if the require()
-// comes from node_modules and reduce the noise. If there are more than 100 frames,
-// just give up and assume it is under node_modules.
-shouldEmitWarning=!isInsideNodeModules(100,true);
+// comes from node_modules as a direct call and reduce the noise.
+shouldEmitWarning=!isInsideNodeModules();
 }
 }else{
 shouldEmitWarning=true;

lib/punycode.js

@@ -3,7 +3,7 @@ const {
  isInsideNodeModules,
 }=internalBinding('util');
 
-if(!isInsideNodeModules(100,true)){
+if(!isInsideNodeModules()){
 process.emitWarning(
 'The `punycode` module is deprecated. Please use a userland '+
 'alternative instead.',

lib/url.js

@@ -132,7 +132,7 @@ const {
 leturlParseWarned=false;
 
 functionurlParse(url,parseQueryString,slashesDenoteHost){
-if(!urlParseWarned&&!isInsideNodeModules(100,true)){
+if(!urlParseWarned&&!isInsideNodeModules(2)){
 urlParseWarned=true;
 process.emitWarning(
 '`url.parse()` behavior is not standardized and prone to '+

src/node_util.cc

@@ -320,23 +320,21 @@ static void GetCallSites(const FunctionCallbackInfo<Value>& args) {
  args.GetReturnValue().Set(callsites);
 }
 
+/**
+ * Checks whether the current call directly initiated from a file inside
+ * node_modules. This checks up to `frame_limit` stack frames, until it finds
+ * a frame that is not part of node internal modules.
+*/
 staticvoidIsInsideNodeModules(const FunctionCallbackInfo<Value>& args) {
  Isolate* isolate = args.GetIsolate();
-CHECK_EQ(args.Length(), 2);
-CHECK(args[0]->IsInt32()); // frame_limit
-// The second argument is the default value.
 
-int frames_limit = args[0].As<v8::Int32>()->Value();
+int frames_limit = (args.Length() > 0 && args[0]->IsInt32())
+ ? args[0].As<v8::Int32>()->Value()
+ : 10;
  Local<StackTrace> stack =
 StackTrace::CurrentStackTrace(isolate, frames_limit);
 int frame_count = stack->GetFrameCount();
 
-// If the search requires looking into more than |frames_limit| frames, give
-// up and return the specified default value.
-if (frame_count == frames_limit) {
-return args.GetReturnValue().Set(args[1]);
- }
-
 bool result = false;
 for (int i = 0; i < frame_count; ++i) {
  Local<StackFrame> stack_frame = stack->GetFrame(isolate, i);
@@ -350,13 +348,11 @@ static void IsInsideNodeModules(const FunctionCallbackInfo<Value>& args) {
 if (script_name_str.starts_with("node:")) {
 continue;
  }
-if (script_name_str.find("/node_modules/") != std::string::npos ||
- script_name_str.find("\\node_modules\\") != std::string::npos ||
- script_name_str.find("/node_modules\\") != std::string::npos ||
- script_name_str.find("\\node_modules/") != std::string::npos) {
- result = true;
-break;
- }
+ result = script_name_str.find("/node_modules/") != std::string::npos ||
+ script_name_str.find("\\node_modules\\") != std::string::npos ||
+ script_name_str.find("/node_modules\\") != std::string::npos ||
+ script_name_str.find("\\node_modules/") != std::string::npos;
+break;
  }
 
  args.GetReturnValue().Set(result);

test/parallel/test-internal-util-isinsidenodemodules.js

@@ -0,0 +1,36 @@
+'use strict';
+
+// Flags: --expose-internals
+
+constcommon=require('../common');
+constassert=require('assert');
+constvm=require('vm');
+
+const{ internalBinding }=require('internal/test/binding');
+const{ isInsideNodeModules }=internalBinding('util');
+
+constscript=newvm.Script(`
+ const runInsideNodeModules = (cb) => {
+ return cb();
+ };
+
+ runInsideNodeModules;
+`,{
+filename: '/workspace/node_modules/test.js',
+});
+construnInsideNodeModules=script.runInThisContext();
+
+// Test when called directly inside node_modules
+assert.strictEqual(runInsideNodeModules(isInsideNodeModules),true);
+
+// Test when called inside a user callback from node_modules
+runInsideNodeModules(common.mustCall(()=>{
+functionnonNodeModulesFunction(){
+assert.strictEqual(isInsideNodeModules(),false);
+}
+
+nonNodeModulesFunction();
+}));
+
+// Test when called outside node_modules
+assert.strictEqual(isInsideNodeModules(),false);

typings/internalBinding/util.d.ts

@@ -45,7 +45,7 @@ export interface UtilBinding {
 guessHandleType(fd: number): 'TCP'|'TTY'|'UDP'|'FILE'|'PIPE'|'UNKNOWN';
 parseEnv(content: string): Record<string,string>;
 styleText(format: Array<string>|string,text: string): string;
-isInsideNodeModules(frameLimit: number,defaultValue: unknown): boolean;
+isInsideNodeModules(frameLimit?: number): boolean;
 constructSharedArrayBuffer(length?: number): SharedArrayBuffer;
 
 constants: {