diff --git a/src/js/internal/util/inspect.js b/src/js/internal/util/inspect.js index 154194ec4f8e..03a8eaa64e4a 100644 --- a/src/js/internal/util/inspect.js +++ b/src/js/internal/util/inspect.js @@ -1781,9 +1781,6 @@ function formatError(err, constructor, tag, ctx, keys) { const name = err.name != null ? String(err.name) : "Error"; let stack = getStackString(err); - //! temp fix for Bun losing the error name from inherited errors + extraneous ": " with no message - stack = stack.replace(/^Error: /, `${name}${err.message ? ": " : ""}`); - removeDuplicateErrorKeys(ctx, keys, err, stack); if ("cause" in err && (keys.length === 0 || !ArrayPrototypeIncludes(keys, "cause"))) { diff --git a/src/jsc/bindings/FormatStackTraceForJS.cpp b/src/jsc/bindings/FormatStackTraceForJS.cpp index d622679f31aa..199984faae80 100644 --- a/src/jsc/bindings/FormatStackTraceForJS.cpp +++ b/src/jsc/bindings/FormatStackTraceForJS.cpp @@ -32,6 +32,60 @@ using namespace WebCore; namespace Bun { +// StackFrame holds cells the GC does not scan from the vector itself. Anything that +// allocates or calls into JS before the frames are formatted can collect them. +static bool protectStackFrameCells(JSC::MarkedArgumentBuffer& protectedFrameCells, WTF::Vector& stackTrace) +{ + protectedFrameCells.ensureCapacity(stackTrace.size() * 2); + for (auto& frame : stackTrace) { + if (auto* callee = frame.callee()) + protectedFrameCells.append(callee); + if (auto* codeBlock = frame.codeBlock()) + protectedFrameCells.append(codeBlock); + } + return !protectedFrameCells.hasOverflowed(); +} + +// V8's ErrorUtils::ToString: [[Get]] "name"/"message" with ToString; undefined defaults to +// "Error" / "". A name/message getter that re-enters stack formatting hits the guard and +// falls back to side-effect-free sanitized reads so the cycle terminates after one level. +static void computeErrorHeader(JSC::VM& vm, Zig::GlobalObject* globalObject, JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSObject* errorObject, WTF::String& name, WTF::String& message) +{ + auto scope = DECLARE_THROW_SCOPE(vm); + name = "Error"_s; + + if (globalObject && globalObject->isComputingErrorStackHeader) { + if (auto* instance = dynamicDowncast(errorObject)) { + name = instance->sanitizedNameString(lexicalGlobalObject); + RETURN_IF_EXCEPTION(scope, ); + message = instance->sanitizedMessageString(lexicalGlobalObject); + RETURN_IF_EXCEPTION(scope, ); + } + return; + } + + if (globalObject) + globalObject->isComputingErrorStackHeader = true; + auto clearFlag = WTF::makeScopeExit([&] { + if (globalObject) + globalObject->isComputingErrorStackHeader = false; + }); + + JSValue nameValue = errorObject->get(lexicalGlobalObject, vm.propertyNames->name); + RETURN_IF_EXCEPTION(scope, ); + if (!nameValue.isUndefined()) { + name = nameValue.toWTFString(lexicalGlobalObject); + RETURN_IF_EXCEPTION(scope, ); + } + + JSValue messageValue = errorObject->get(lexicalGlobalObject, vm.propertyNames->message); + RETURN_IF_EXCEPTION(scope, ); + if (!messageValue.isUndefined()) { + message = messageValue.toWTFString(lexicalGlobalObject); + RETURN_IF_EXCEPTION(scope, ); + } +} + static JSValue formatStackTraceToJSValue(JSC::VM& vm, Zig::GlobalObject* globalObject, JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSObject* errorObject, JSC::JSArray* callSites) { auto scope = DECLARE_THROW_SCOPE(vm); @@ -41,21 +95,18 @@ static JSValue formatStackTraceToJSValue(JSC::VM& vm, Zig::GlobalObject* globalO WTF::StringBuilder sb; - auto errorMessage = errorObject->getIfPropertyExists(lexicalGlobalObject, vm.propertyNames->message); + WTF::String name; + WTF::String message; + computeErrorHeader(vm, globalObject, lexicalGlobalObject, errorObject, name, message); RETURN_IF_EXCEPTION(scope, {}); - if (errorMessage) { - auto* str = errorMessage.toString(lexicalGlobalObject); - RETURN_IF_EXCEPTION(scope, {}); - if (str->length() > 0) { - auto value = str->view(lexicalGlobalObject); - RETURN_IF_EXCEPTION(scope, {}); - sb.append("Error: "_s); - sb.append(value.data); - } else { - sb.append("Error"_s); + if (!name.isEmpty()) { + sb.append(name); + if (!message.isEmpty()) { + sb.append(": "_s); + sb.append(message); } - } else { - sb.append("Error"_s); + } else if (!message.isEmpty()) { + sb.append(message); } for (size_t i = 0; i < framesCount; i++) { @@ -408,23 +459,21 @@ static String computeErrorInfoWithoutPrepareStackTrace( WTF::String name = "Error"_s; WTF::String message; - if (errorInstance) { - // Note that we are not allowed to allocate memory in here. It's called inside a finalizer. - if (auto* instance = dynamicDowncast(errorInstance)) { - if (!lexicalGlobalObject) { - lexicalGlobalObject = errorInstance->globalObject(); - } - name = instance->sanitizedNameString(lexicalGlobalObject); - RETURN_IF_EXCEPTION(scope, {}); - message = instance->sanitizedMessageString(lexicalGlobalObject); - RETURN_IF_EXCEPTION(scope, {}); - } - } - if (!globalObject) [[unlikely]] { globalObject = defaultGlobalObject(); } + if (errorInstance) { + // The GC-finalizer path (computeErrorInfoWrapperToString) always passes a null + // errorInstance, so this branch only runs from a mutator (lazy .stack getter, + // materializeErrorInfoIfNeeded, captureStackTrace) where user code may execute. + if (!lexicalGlobalObject) { + lexicalGlobalObject = errorInstance->globalObject(); + } + computeErrorHeader(vm, globalObject, lexicalGlobalObject, errorInstance, name, message); + RETURN_IF_EXCEPTION(scope, {}); + } + return Bun::formatStackTrace(vm, globalObject, lexicalGlobalObject, name, message, line, column, sourceURL, stackTrace, errorInstance); } @@ -719,6 +768,17 @@ JSC_DEFINE_CUSTOM_GETTER(errorInstanceLazyStackCustomGetter, (JSGlobalObject * g String sourceURL; auto stackTrace = errorObject->stackTrace(); + // A name/message getter reading .stack re-enters here while the outer materialize still + // holds a &*m_stackTrace; moving/reassigning it would free that Vector under the outer + // call. Under the guard computeErrorHeader uses sanitized reads, so no user code runs. + auto* zigGlobalObject = defaultGlobalObject(globalObject); + if (stackTrace && zigGlobalObject && zigGlobalObject->isComputingErrorStackHeader) [[unlikely]] { + JSValue result = computeErrorInfoToJSValue(vm, *stackTrace, line, column, sourceURL, errorObject, nullptr); + RETURN_IF_EXCEPTION(scope, {}); + errorObject->putDirect(vm, vm.propertyNames->stack, result, JSC::PropertyAttribute::DontEnum | 0); + return JSValue::encode(result); + } + JSValue result; if (stackTrace == nullptr) { WTF::Vector emptyTrace; @@ -726,14 +786,7 @@ JSC_DEFINE_CUSTOM_GETTER(errorInstanceLazyStackCustomGetter, (JSGlobalObject * g } else { auto ownedStackTrace = makeUnique>(WTF::move(*stackTrace)); JSC::MarkedArgumentBuffer protectedFrameCells; - protectedFrameCells.ensureCapacity(ownedStackTrace->size() * 2); - for (auto& frame : *ownedStackTrace) { - if (auto* callee = frame.callee()) - protectedFrameCells.append(callee); - if (auto* codeBlock = frame.codeBlock()) - protectedFrameCells.append(codeBlock); - } - if (protectedFrameCells.hasOverflowed()) [[unlikely]] { + if (!protectStackFrameCells(protectedFrameCells, *ownedStackTrace)) [[unlikely]] { throwOutOfMemoryError(globalObject, scope); return {}; } @@ -778,6 +831,15 @@ JSC_DEFINE_HOST_FUNCTION(errorConstructorFuncCaptureStackTrace, (JSC::JSGlobalOb WTF::Vector stackTrace; JSCStackTrace::getFramesForCaller(vm, callFrame, errorObject, caller, stackTrace, stackTraceLimit); + // Both eager-compute paths below read name/message via [[Get]] before formatting, + // which may allocate or run a user getter. The lazy path moves the frames into the + // ErrorInstance, which visits them; rooting is a no-op there. + JSC::MarkedArgumentBuffer protectedFrameCells; + if (!protectStackFrameCells(protectedFrameCells, stackTrace)) [[unlikely]] { + throwOutOfMemoryError(globalObject, scope); + return {}; + } + if (auto* instance = dynamicDowncast(errorObject)) { if (instance->hasMaterializedErrorInfo()) { // Error info was already materialized (e.g. .stack was previously accessed). diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index ca759a74da9e..67ba9088aae2 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -434,6 +434,7 @@ class GlobalObject : public Bun::GlobalScope { bool asyncHooksNeedsCleanup = false; double INSPECT_MAX_BYTES = 50; bool isInsideErrorPrepareStackTraceCallback = false; + bool isComputingErrorStackHeader = false; template using LazyPropertyOfGlobalObject = LazyProperty; diff --git a/test/js/node/util/node-inspect-tests/parallel/util-format.test.js b/test/js/node/util/node-inspect-tests/parallel/util-format.test.js index 60b624034f36..ec277964bf84 100644 --- a/test/js/node/util/node-inspect-tests/parallel/util-format.test.js +++ b/test/js/node/util/node-inspect-tests/parallel/util-format.test.js @@ -419,7 +419,7 @@ test("no assertion failures", () => { // Errors const err = new Error("foo"); - assert(util.format(err).startsWith(err.stack), `Expected "${util.format(err)}" to start with "${err.stack}"`); + assert.strictEqual(util.format(err), err.stack); class CustomError extends Error { constructor(msg) { @@ -433,7 +433,7 @@ test("no assertion failures", () => { customError.stack; delete customError.originalLine; delete customError.originalColumn; - assert.strictEqual(util.format(customError), customError.stack.replace(/^Error/, "Custom$&")); //! temp bug workaround + assert.strictEqual(util.format(customError), customError.stack); // Doesn't capture stack trace function BadCustomError(msg) { Error.call(this); diff --git a/test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js b/test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js index e538109fff99..d1aae6b2e23a 100644 --- a/test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js +++ b/test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js @@ -591,14 +591,7 @@ test("no assertion failures 2", () => { // Exceptions should print the error message, not '{}'. { [new Error(), new Error("FAIL"), new TypeError("FAIL"), new SyntaxError("FAIL")].forEach(err => { - assert( - //! temp bug workaround with replace()'s - util.inspect(err).startsWith(err.stack.replace(/^Error: /, err.message ? "$&" : "Error")), - `Expected "${util.inspect(err)}" to start with "${err.stack.replace( - /^Error: /, - err.message ? "$&" : "Error", - )}"`, - ); + assert.strictEqual(util.inspect(err), err.stack); }); assert.throws( @@ -1854,11 +1847,10 @@ test("no assertion failures 3", () => { ].forEach(([Class, message], i) => { const foo = new Class(message); const extra = Class.name.includes("Error") ? "" : ` [${foo.name}]`; - // TODO: Bun messes with `Error.stack` and this causes this to fail - // assert( - // util.inspect(foo).startsWith(`${Class.name}${extra}${message ? `: ${message}` : "\n"}`), - // util.inspect(foo) + "\n...did not start with: " + `${Class.name}${extra}${message ? `: ${message}` : "\n"}`, - // ); + assert( + util.inspect(foo).startsWith(`${Class.name}${extra}${message ? `: ${message}` : "\n"}`), + util.inspect(foo) + "\n...did not start with: " + `${Class.name}${extra}${message ? `: ${message}` : "\n"}`, + ); Object.defineProperty(foo, Symbol.toStringTag, { value: "WOW", writable: true, @@ -1871,11 +1863,10 @@ test("no assertion failures 3", () => { `Expected to start with: "[This is a stack]"\nFound: "${util.inspect(foo)}"`, ); foo.stack = stack; - // TODO: Bun messes with `Error.stack` and this causes this to fail - // assert( - // util.inspect(foo).startsWith(`${Class.name} [WOW]${extra}${message ? `: ${message}` : "\n"}`), - // util.inspect(foo), - // ); + assert( + util.inspect(foo).startsWith(`${Class.name} [WOW]${extra}${message ? `: ${message}` : "\n"}`), + util.inspect(foo), + ); Object.setPrototypeOf(foo, null); assert( util.inspect(foo).startsWith( @@ -3189,6 +3180,164 @@ test("no assertion failures 3", () => { } }); +test("error inspect preserves stack header when name/message change after materialization", () => { + // Bun's native .stack already emits `${name}${message ? ": " + message : ""}` as the first line, + // so formatError must not rewrite it. These headers match Node's output for the same inputs. + const firstLine = e => util.inspect(e).split("\n")[0]; + + // message cleared after .stack was materialized: header is preserved verbatim + { + const err = new Error("msg"); + void err.stack; + err.message = ""; + assert.strictEqual(firstLine(err), "Error: msg"); + } + { + const err = new Error("Error: nested"); + void err.stack; + err.message = ""; + assert.strictEqual(firstLine(err), "Error: Error: nested"); + } + + // user-assigned stack starting with "Error: " on an empty-message Error is preserved + { + const err = new Error(); + err.stack = "Error: manually set\n at foo"; + assert.strictEqual(firstLine(err), "Error: manually set"); + } + + // name changed after .stack was materialized: header is not rewritten to the new name + { + const err = new Error("x"); + void err.stack; + err.name = "Renamed"; + assert.strictEqual(firstLine(err), "Error: x"); + } + + // native header is correct for subclassed errors and empty-message errors + { + class Foo extends Error { + name = "Foo"; + } + const err = new Foo("x"); + assert.strictEqual(err.stack.split("\n")[0], "Foo: x"); + assert.strictEqual(firstLine(err), "Foo: x"); + } + { + const err = new Error(); + assert.strictEqual(err.stack.split("\n")[0], "Error"); + assert.strictEqual(firstLine(err), "Error"); + } +}); + +test("error stack header reads name/message via full [[Get]]", () => { + // V8 composes the .stack header with ErrorUtils::ToString: ordinary [[Get]] on "name" + // and "message" (prototype walk, accessors, ToString). Each expected value matches Node. + const header = e => e.stack.split("\n")[0]; + const inspected = e => util.inspect(e).split("\n")[0]; + + // name on an intermediate prototype (2+ levels deep) + { + class Bar extends Error {} + class Foo extends Bar {} + Bar.prototype.name = "Bar"; + const err = new Foo("x"); + assert.strictEqual(header(err), "Bar: x"); + assert.strictEqual(inspected(err), "Bar: x"); + } + + // name defined as an accessor + { + class G extends Error { + get name() { + return "G"; + } + } + const err = new G("m"); + assert.strictEqual(header(err), "G: m"); + assert.strictEqual(inspected(err), "G: m"); + } + + // non-primitive name coerces via ToString + { + const err = new Error("m"); + err.name = { toString: () => "O" }; + assert.strictEqual(header(err), "O: m"); + } + + // message defined as an accessor / on an intermediate prototype + { + class M extends Error { + get message() { + return "acc-msg"; + } + } + assert.strictEqual(header(new M()), "Error: acc-msg"); + } + { + class A extends Error {} + class B extends A {} + A.prototype.message = "deep"; + assert.strictEqual(header(new B()), "Error: deep"); + } + + // undefined name defaults to "Error"; null name stringifies + { + const e1 = new Error("m"); + e1.name = undefined; + assert.strictEqual(header(e1), "Error: m"); + const e2 = new Error("m"); + e2.name = null; + assert.strictEqual(header(e2), "null: m"); + } + + // a name getter that throws propagates out of the .stack read + { + class T extends Error { + get name() { + throw new TypeError("name-boom"); + } + } + assert.throws(() => new T("m").stack, /name-boom/); + } + + // the Error.prepareStackTrace default string uses the same header + { + const saved = Error.prepareStackTrace; + try { + Error.prepareStackTrace = (e, s) => e.stack; + class Foo extends Error { + name = "Foo"; + } + assert.strictEqual(header(new Foo("x")), "Foo: x"); + assert.strictEqual(inspected(new Foo("x")), "Foo: x"); + } finally { + Error.prepareStackTrace = saved; + } + } + + // a name/message getter that calls Error.captureStackTrace(this) does not recurse unboundedly + { + let count = 0; + class M extends Error { + get message() { + count++; + Error.captureStackTrace(this); + return "m"; + } + } + assert.strictEqual(header(new M()), "Error: m"); + assert.strictEqual(count, 1); + } + + // Error.captureStackTrace on a non-Error target uses its name and message + { + const target = { name: "X", message: "Y" }; + Error.captureStackTrace(target); + assert.strictEqual(target.stack.split("\n")[0], "X: Y"); + } +}); + // Utility functions function runCallChecks(exitCode) { if (exitCode !== 0) return; diff --git a/test/js/node/v8/capture-stack-trace.test.js b/test/js/node/v8/capture-stack-trace.test.js index 6cd46a1ad90a..bb2ce68ca2d5 100644 --- a/test/js/node/v8/capture-stack-trace.test.js +++ b/test/js/node/v8/capture-stack-trace.test.js @@ -1121,3 +1121,27 @@ test("lazy error-info materialization does not store an empty stack value when t }); expect(exitCode).toBe(0); }); + +test("a name getter that reads .stack while the lazy captureStackTrace accessor materializes does not free m_stackTrace under the outer call", async () => { + // WTF::Vector goes through bmalloc; Malloc=1 routes it to system malloc so ASAN sees it. + const src = ` + let n = 0; + class T extends Error { get name() { if (n++ === 0) void this.stack; return "T"; } } + const e = new T("m"); + Error.captureStackTrace(e); + const outer = e.stack; + console.log(JSON.stringify({ header: String(outer).split("\\n")[0], frames: String(outer).split("\\n").length > 1 })); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", src], + env: { ...bunEnv, Malloc: "1" }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), signalCode: proc.signalCode }).toEqual({ + stdout: JSON.stringify({ header: "T: m", frames: true }), + signalCode: null, + }); + expect(exitCode).toBe(0); +});