Skip to content
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.

Sign up for GitHub

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

src,lib: fix assert when Error.prepareStackTrace not overridden #49212

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −4
Open

src,lib: fix assert when Error.prepareStackTrace not overridden #49212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions lib/.eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ rules:
message: Use an error exported by the internal/errors module.
- selector: CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']
message: Please use `require('internal/errors').hideStackFrames()` instead.
- selector: AssignmentExpression:matches([left.name='prepareStackTrace'], [left.property.name='prepareStackTrace'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of removing this, you can add // eslint-disable-next-line no-restricted-syntax

message: Use 'overrideStackTrace' from 'lib/internal/errors.js' instead of 'Error.prepareStackTrace'.
- selector: ThrowStatement > NewExpression[callee.name=/^ERR_[A-Z_]+$/] > ObjectExpression:first-child:not(:has([key.name='message']):has([key.name='code']):has([key.name='syscall']))
message: The context passed into SystemError constructor must have .code, .syscall and .message.
no-restricted-globals:
Expand Down
13 changes: 11 additions & 2 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const { openSync, closeSync, readSync } = require('fs');
const { inspect } = require('internal/util/inspect');
const { isPromise, isRegExp } = require('internal/util/types');
const { EOL } = require('internal/constants');
const { getEmbedderOptions } = require('internal/options');
const { BuiltinModule } = require('internal/bootstrap/realm');
const { isError, deprecate } = require('internal/util');

Expand Down Expand Up @@ -293,8 +294,16 @@ function getErrMessage(message, fn) {
ErrorCaptureStackTrace(err, fn);
if (errorStackTraceLimitIsWritable) Error.stackTraceLimit = tmpLimit;

overrideStackTrace.set(err, (_, stack) => stack);
const call = err.stack[0];
let call;
if (getEmbedderOptions().hasPrepareStackTraceCallback) {
overrideStackTrace.set(err, (_, stack) => stack);
call = err.stack[0];
} else {
const tmpPrepare = Error.prepareStackTrace;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm actually I think both blocks are assuming env->prepare_stack_trace_callback() would be called here? Error.prepareStackTrace doesn't even do anything if env->prepare_stack_trace_callback() is not configured (it's a polyfill for that hook).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work: https://github.com/v8/v8/blob/main/src/execution/messages.cc#L325-L335 What we polyfill in nodejs is that Error.prepareStackTrace in the main realm will affect stack traces in any other realm.

Copy link
Member

@joyeecheung joyeecheung Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah didn't realize that V8 has not removed it yet..in that case I wonder if instead of using the WeakMap, we can just switch to storing a v8::Private::ForApi() to the error to indicate that the prepare stack trace callback should use the function stored with that symbol to override the stack trace? Then the embedder can also communicate with the Node.js internals using v8::Private::ForApi() without requiring access to the overrideStackTrace map - overrideStackTrace isn't only used here, even, and WeakMap semantics means using v8::Private::ForApi() would just be equivalent. That would also avoid the Error.prepareStackTrace configurability/writability problem.

Error.prepareStackTrace = (_, stack) => stack;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would throw if Error.prepareStackTrace is not writable, not sure if that's desirable.

call = err.stack[0];
Error.prepareStackTrace = tmpPrepare;
}

const filename = call.getFileName();
const line = call.getLineNumber() - 1;
Expand Down
3 changes: 3 additions & 0 deletions src/api/environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,9 @@ void SetIsolateErrorHandlers(v8::Isolate* isolate, const IsolateSettings& s) {
auto* prepare_stack_trace_cb = s.prepare_stack_trace_callback ?
s.prepare_stack_trace_callback : PrepareStackTraceCallback;
isolate->SetPrepareStackTraceCallback(prepare_stack_trace_cb);
} else {
auto env = Environment::GetCurrent(isolate);
env->set_prepare_stack_trace_callback(Local<Function>());
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,13 @@ void GetEmbedderOptions(const FunctionCallbackInfo<Value>& args) {
Local<Context> context = env->context();
Local<Object> ret = Object::New(isolate);

if (ret->Set(context,
FIXED_ONE_BYTE_STRING(
env->isolate(), "hasPrepareStackTraceCallback"),
Boolean::New(isolate,
!env->prepare_stack_trace_callback().IsEmpty()))
Copy link
Member

@joyeecheung joyeecheung Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overridden when the realm is bootstrapped though, unless SetIsolateUpForNode() is somehow called after CreateEnvironment() this would not have worked?

node/lib/internal/bootstrap/realm.js

Line 449 in fe219e0

setPrepareStackTraceCallback(prepareStackTrace);

I wonder if this flag should've been moved into EnvironmentFlags instead (and if that's too breaking, keep SHOULD_NOT_SET_PREPARE_STACK_TRACE_CALLBACK a dummy flag there), and the GetEmbedderOptions() could query that flag stored into the Environment instead. At bootstrap time, we could just skip setPrepareStackTraceCallback if that flag is true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung ah, we do call CreateEnvironment() before SetIsolateUpForNode() - I can switch that around.

.IsNothing()) return;

if (ret->Set(context,
FIXED_ONE_BYTE_STRING(env->isolate(), "shouldNotRegisterESMLoader"),
Boolean::New(isolate, env->should_not_register_esm_loader()))
Expand Down