src,lib: fix assert when Error.prepareStackTrace not overridden
#49212
Conversation
codebytere
added
the
embedding
nodejs-github-bot
added
assert
codebytere
force-pushed
the
fix-handle-no-override-preparestacktrace
branch
from
40a5d30 to
661bc35
Compare
codebytere
force-pushed
the
fix-handle-no-override-preparestacktrace
branch
from
661bc35 to
151654e
Compare
| FIXED_ONE_BYTE_STRING( | ||
| env->isolate(), "hasPrepareStackTraceCallback"), | ||
| Boolean::New(isolate, | ||
| !env->prepare_stack_trace_callback().IsEmpty())) |
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
@joyeecheung ah, we do call CreateEnvironment() before SetIsolateUpForNode() - I can switch that around.
| overrideStackTrace.set(err, (_, stack) => stack); | ||
| call = err.stack[0]; | ||
| } else { | ||
| const tmpPrepare = Error.prepareStackTrace; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| call = err.stack[0]; | ||
| } else { | ||
| const tmpPrepare = Error.prepareStackTrace; | ||
| Error.prepareStackTrace = (_, stack) => stack; |
There was a problem hiding this comment.
This would throw if Error.prepareStackTrace is not writable, not sure if that's desirable.
| @@ -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']) | |||
There was a problem hiding this comment.
instead of removing this, you can add // eslint-disable-next-line no-restricted-syntax
| overrideStackTrace.set(err, (_, stack) => stack); | ||
| call = err.stack[0]; | ||
| } else { | ||
| const tmpPrepare = Error.prepareStackTrace; |
There was a problem hiding this comment.
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.
Refs #23926
Fixes an issue caused when embedders choose to set up the
v8::Isolatewithout a prepareStackTrace callback. In this event, severalassertmethods stop working because stack trace formatting relies on aSafeWeakMapthat's only populated in the overriddenprepareStackTracecallback.Fix this by exposing override status under
getEmbedderOptionsand falling back to the default implementation ofError.prepareStackTraceif it's not been overridden.