Repository navigation
Conversation
With a host that formats stacks (vm.onComputeErrorInfoJSValue(); Bun), an
ErrorInstance gets its stack property when materializeErrorInfoIfNeeded()
finds frames or a stack string to format. An error that has neither never
gets one. That is every error made while Error.stackTraceLimit is 0,
negative or not a number, and every error that native code makes while no
JavaScript is on the stack.
Error.stackTraceLimit = 0;
const error = new Error("m");
error.stack; // undefined, "Error: m" in V8
error.stack = "value";
Object.keys(error); // ["stack"], [] in V8
In V8 every error owns a non-enumerable stack. Its value is the header
line when a stack was captured with no frame in it, and undefined when no
stack was captured (the limit was not a number).
The first lookup of such an error now installs a lazy stack: a
non-enumerable custom value, ErrorInstance::installLazyStack(). Nothing is
formatted for it. JSObject::put replaces a custom value with the stored
value, and a delete removes it, without a call of the getter. So a store,
a delete, hasOwnProperty and the own keys run no code of the host or of
the user. A read formats the stack: the host's formatter gets an empty
frame list, so Error.prepareStackTrace gets no call site, and from then on
the error info is materialized. An error that captured no stack reads
undefined, and the host is not asked. m_lazyStackInstalled keeps a stack
that was deleted from coming back.
The getter is errorInstanceMaterializingStackGetter, the one of the stack
that Error.captureStackTrace() saves. Both properties are custom values
with the same Structure, and a compiled lookup takes the getter from the
Structure, so the two cannot have a getter each. The getter first
materializes what there is to materialize, as before. That also formats
frames that the host attached after the lazy stack was installed
(setStackFrames()).
Every error with a lazy stack holds the same CustomGetterSetter, which the
VM keeps.
Nothing changes without the host's formatter. The jsc shell does not set
one, so JSTests/stress/error-stack-trace-limit.js still reads undefined.
* Source/JavaScriptCore/runtime/Error.cpp:
(JSC::addErrorInfo):
* Source/JavaScriptCore/runtime/ErrorInstance.cpp:
(JSC::JSC_DEFINE_CUSTOM_GETTER):
(JSC::lazyStackGetterSetter):
(JSC::ErrorInstance::ErrorInstance):
(JSC::ErrorInstance::materializeErrorInfoIfNeeded):
(JSC::ErrorInstance::installLazyStack):
(JSC::ErrorInstance::materializeLazyStack):
* Source/JavaScriptCore/runtime/ErrorInstance.h:
* Source/JavaScriptCore/runtime/VM.cpp:
(JSC::VM::visitAggregateImpl):
* Source/JavaScriptCore/runtime/VM.h:
|
Preview build of 7aa6826: |
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft until Bun's tests pass against this PR's preview build.
Problem
ErrorInstancewith no frame to format never ownsstack.error.stackisundefined, anderror.stack = valueadds an enumerable key. In V8 every error owns a non-enumerablestack.materializeErrorInfoIfNeeded()(ErrorInstance.cpp:530) definesstackonly for frames or a stack string. An error has neither underError.stackTraceLimit = 0, under a limit that is not a number, or when native code makes it with no JavaScript on the stack.Fix
stack: a non-enumerable custom value (installLazyStack()). A store replaces it and a delete removes it. Neither formats.undefinedif the error captured no stack. Both as in V8.errorInstanceMaterializingStackGetter: a compiled lookup takes the getter from the Structure, and both properties have the same one.vm.onComputeErrorInfoJSValue(). Thejscshell sets none.Background
CustomGetterSetterand reads through its getter. To JavaScript it is a data property.Object.freeze(). Formatting on every lookup runsError.prepareStackTraceon a store.Downsides
undefined, andError.prepareStackTraceis called for it.Error: m123. The first lookup allocates nothing.defineProperty(),freeze()andseal()format too: 11 of 26 lookup kinds, where V8 has 5.Notes
ErrorInstance.cpplinked in front of the pinnedlibJavaScriptCore.a.test/js/node/v8/capture-stack-trace.test.jspasses there (22 new tests). The run against this PR's preview build is pending.m_lazyStackInstalledis set by the first lookup, also when the error already owns astackthat native code put there withputDirect. A stack that is deleted does not come back, as in V8:delete error.stack; error.stack = 1gives an ordinary enumerable property.m_errorInfoMaterializedand drops the empty frame list. A host that attaches frames later (setStackFrames(), Bun's async stack attach) checkshasMaterializedErrorInfo()first, so a stack that was read does not change. Frames that were attached before the first read are formatted by the read: the getter callsmaterializeErrorInfoIfNeeded()first.Error.prepareStackTracegets an empty call-site array for such an error, on the read only.Error.captureStackTrace(anode:vmcontext in Bun). An error with a lazy stack and an error with a saved captured stack have the same Structure there. With a getter each, a compiled read of one called the getter of the other.CustomGetterSetter, which the VM keeps: 10,000 such errors add 1 cell and 1 Structure.sizeof(ErrorInstance)stays 72. The new bit is in the existing bit-field.Error.stackTraceLimit = NaNis unchanged:JSGlobalObject::stackTraceLimit()gives no limit, the error captures no stack, and its stack readsundefined(V8: the header).Error.prepareStackTrace, of 26 probed: V8 5, this PR 11, before 0, an error with frames 20.JSTestscase: thejscshell installs no stack formatter, so the new path does not run there.JSTests/stress/error-stack-trace-limit.jsanderror-stack-trace-limit-nan.jscover that nothing changes without one.Promise.anychange, with the stack formatted on any lookup. This PR replaces that half.