Skip to content

Commit

Permalink
src: implement IsInsideNodeModules() in C++
Browse files Browse the repository at this point in the history
This previously compiles a script and run it in a new context
to avoid global pollution, which is more complex than necessary
and can be too slow for it to be reused in other cases. The
new implementation just checks the frames in C++ which is safe
from global pollution, faster and simpler.

The previous implementation also had a bug when the call site
is in a ESM, because ESM have URLs as their script names,
which don't start with '/' or '\' and will be skipped. The new
implementation removes the skipping to fix it for ESM.
  • Loading branch information
joyeecheung committed Oct 5, 2024
1 parent 20d8b85 commit 5273982
Show file tree
Hide file tree
Showing 10 changed files with 107 additions and 31 deletions.
6 changes: 4 additions & 2 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@ const {
ONLY_ENUMERABLE,
},
getOwnNonIndexProperties,
isInsideNodeModules,
} = internalBinding('util');
const {
customInspectSymbol,
isInsideNodeModules,
lazyDOMException,
normalizeEncoding,
kIsEncodingSymbol,
Expand Down Expand Up @@ -178,13 +178,15 @@ function showFlaggedDeprecation() {
if (bufferWarningAlreadyEmitted ||
++nodeModulesCheckCounter > 10000 ||
(!require('internal/options').getOptionValue('--pending-deprecation') &&
isInsideNodeModules())) {
isInsideNodeModules(100, true))) {
// 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.
return;
}

Expand Down
26 changes: 0 additions & 26 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -516,31 +516,6 @@ function getStructuredStack() {
return getStructuredStackImpl();
}

function isInsideNodeModules() {
const stack = getStructuredStack();

// Iterate over all stack frames and look for the first one not coming
// from inside Node.js itself:
if (ArrayIsArray(stack)) {
for (const frame of stack) {
const filename = frame.getFileName();

if (
filename == null ||
StringPrototypeStartsWith(filename, 'node:') === true ||
(
filename[0] !== '/' &&
StringPrototypeIncludes(filename, '\\') === false
)
) {
continue;
}
return isUnderNodeModules(filename);
}
}
return false;
}

function once(callback, { preserveReturnValue = false } = kEmptyObject) {
let called = false;
let returnValue;
Expand Down Expand Up @@ -914,7 +889,6 @@ module.exports = {
getSystemErrorName,
guessHandleType,
isError,
isInsideNodeModules,
isUnderNodeModules,
isMacOS,
isWindows,
Expand Down
46 changes: 43 additions & 3 deletions src/node_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,47 @@ static void GetCallSite(const FunctionCallbackInfo<Value>& args) {

callsite_objects.push_back(obj);
}
}

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();
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]);
}

Local<Array> callsites =
Array::New(isolate, callsite_objects.data(), callsite_objects.size());
args.GetReturnValue().Set(callsites);
bool result = false;
for (int i = 0; i < frame_count; ++i) {
Local<StackFrame> stack_frame = stack->GetFrame(isolate, i);
Local<String> script_name = stack_frame->GetScriptName();

if (script_name.IsEmpty() || script_name->Length() == 0) {
continue;
}

std::string script_name_str = Utf8Value(isolate, script_name).ToString();
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;
}
}

args.GetReturnValue().Set(result);
}

void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
Expand All @@ -313,6 +350,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(FastGuessHandleType);
registry->Register(fast_guess_handle_type_.GetTypeInfo());
registry->Register(ParseEnv);
registry->Register(IsInsideNodeModules);
}

void Initialize(Local<Object> target,
Expand Down Expand Up @@ -396,6 +434,8 @@ void Initialize(Local<Object> target,
target->Set(context, env->constants_string(), constants).Check();
}

SetMethod(
context, target, "isInsideNodeModules", IsInsideNodeModules);
SetMethodNoSideEffect(
context, target, "getPromiseDetails", GetPromiseDetails);
SetMethodNoSideEffect(context, target, "getProxyDetails", GetProxyDetails);
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/warning_node_modules/new-buffer-cjs.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/warning_node_modules/new-buffer-esm.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions test/parallel/test-buffer-constructor-node-modules.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
'use strict';

const common = require('../common');
const fixtures = require('../common/fixtures');
const { spawnSyncAndAssert } = require('../common/child_process');

if (process.env.NODE_PENDING_DEPRECATION)
common.skip('test does not work when NODE_PENDING_DEPRECATION is set');

spawnSyncAndAssert(
process.execPath,
[ fixtures.path('warning_node_modules', 'new-buffer-cjs.js') ],
{
trim: true,
stderr: '',
}
);

spawnSyncAndAssert(
process.execPath,
[ fixtures.path('warning_node_modules', 'new-buffer-esm.mjs') ],
{
trim: true,
stderr: '',
}
);

spawnSyncAndAssert(
process.execPath,
[
'--pending-deprecation',
fixtures.path('warning_node_modules', 'new-buffer-cjs.js')

Check failure on line 32 in test/parallel/test-buffer-constructor-node-modules.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Missing trailing comma
],
{
stderr: /DEP0005/
}
);

spawnSyncAndAssert(
process.execPath,
[
'--pending-deprecation',
fixtures.path('warning_node_modules', 'new-buffer-esm.mjs')

Check failure on line 43 in test/parallel/test-buffer-constructor-node-modules.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Missing trailing comma
],
{
stderr: /DEP0005/
}
);

0 comments on commit 5273982

Please sign in to comment.