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

PR-URL: #60991
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

Author: legendecas

benchmark/internal/util_isinsidenodemodules.js

@@ -4,14 +4,13 @@ const assert = require('assert');
 
 const bench = common.createBenchmark(main, {
   n: [1e6],
-  stackLimit: [100],
   stackCount: [99, 101],
   method: ['isInsideNodeModules', 'noop'],
 }, {
   flags: ['--expose-internals', '--disable-warning=internal/test/binding'],
 });
 
-function main({ n, stackLimit, stackCount, method }) {
+function main({ 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 (let i = 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) {
         } else if (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 {
 let urlParseWarned = false;
 
 function urlParse(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.
+ */
 static void IsInsideNodeModules(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
+
+const common = require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+const { internalBinding } = require('internal/test/binding');
+const { isInsideNodeModules } = internalBinding('util');
+
+const script = new vm.Script(`
+  const runInsideNodeModules = (cb) => {
+    return cb();
+  };
+
+  runInsideNodeModules;
+`, {
+  filename: '/workspace/node_modules/test.js',
+});
+const runInsideNodeModules = 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(() => {
+  function nonNodeModulesFunction() {
+    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: {