ErrorInstance: report "not found" when the error info callback throws in getOwnPropertySlot and deleteProperty

[robobun]

Problem

• ErrorInstance::materializeErrorInfoIfNeeded calls VM::onComputeErrorInfoJSValue() (a Bun addition). In Bun that callback runs Error.prepareStackTrace, so it can throw. The function stores stack, line and column in both cases.
• ErrorInstance::getOwnPropertySlot (ErrorInstance.cpp:511) then finds the stored property and returns true with the exception pending. A debug build of Bun aborts:

  ASSERTION FAILED: !scope.exception() || !result
  vendor/WebKit/Source/JavaScriptCore/runtime/JSObject.cpp(3958) : bool JSC::JSObject::getOwnPropertyDescriptor(JSGlobalObject *, PropertyName, PropertyDescriptor &)
  

  Repro: Error.prepareStackTrace = () => { throw new Error("hook-throw") }; Object.getOwnPropertyDescriptor(new Error("x"), "stack").
• ErrorInstance::deleteProperty has the same gap: it deletes with the exception pending.

Fix

• Both functions return false when this call created the lazy properties and an exception is pending. The exception still reaches the caller.
• They read VM::exceptionForInspection() and declare no ThrowScope, as setUpStaticFunctionSlot does since Drop the ThrowScope from the reifyStaticProperty call sites #306. A ThrowScope here makes validateExceptionChecks=1 abort in callers that never check, such as JSModuleLoader::isFetchError.
• One release result changes: a throwing delete error.stack no longer removes stack, as with put and defineOwnProperty.
• Verified: Bun's debug build on the preview autobuild-preview-pr-687-740d34a2 passes the 4 new tests of Fix a debug assertion when Error.prepareStackTrace throws during getOwnPropertyDescriptor on an error (WebKit bump) bun#43096. All 4 fail on the current pin. The jsc shell cannot set the callback, so the tests are in Bun.

Background

• An ErrorInstance creates stack, line, column and sourceURL on the first touch of any of them. Upstream builds a string there and cannot throw. The fork added an exception check to defineOwnProperty and put only.
• A getOwnPropertySlot that throws must return false. EXCEPTION_ASSERT checks that in debug builds only.
• validateExceptionChecks=1 makes each ThrowScope destructor record a simulated throw, and aborts if the caller does not check. Bun's ASAN lane runs most tests with it.

Notes

Why no ThrowScope. The first version of this PR declared one at the top of getOwnPropertySlot, as JSFunction::getOwnPropertySlot does. With validateExceptionChecks=1, an entry module with await 1; throw new Error() then aborted: "This scope can throw a JS exception: getOwnPropertySlot @ ErrorInstance.cpp:514 ... unchecked as of this scope: runInternalMicrotask". JSModuleLoader::isFetchError and maybeDuplicateFetchError call hasOwnProperty() on an error with a private name and do not check afterwards, which is correct because that lookup cannot throw. With this version and the validator on, these pass in Bun: capture-stack-trace.test.js (61), structured-clone.test.ts (235), type-export.test.ts (70), worker-top-level-await.test.ts (6), and nine module scripts (a throw after a top-level await, a failed import, a link failure, a syntax error, a require that throws).

Found by fuzzing a debug build of Bun. No user reported it, and only a build with assertions aborts. oven-sh/bun#34104 (merged) already names this gap and says that the proper fix is a check in ErrorInstance::getOwnPropertySlot on the WebKit side. oven-sh/bun#34095 shows the same assertion text, but its cause was the lazy process properties, so it is not a report of this path.

A termination is the same case with no throw in user code. worker.terminate() while the worker is inside the callback leaves a TerminationException pending, and the descriptor lookup aborts the same way. The tests in oven-sh/bun#43096 cover it with a callback that loops until the worker is terminated. On the current pin that crashes on every run.

Operations that abort a debug build of Bun (main at 812799ce8c, WebKit c28156899e) with a throwing Error.prepareStackTrace, out of about 115 that were tried on a fresh new Error("x"):

• Object.getOwnPropertyDescriptor(e, "stack"), and the same for "line" and "column"
• Reflect.getOwnPropertyDescriptor(e, "stack")
• e.propertyIsEnumerable("stack")
• Object.getOwnPropertyDescriptor(new Proxy(e, {}), "stack")

All six reach the assertion through JSObject::getOwnPropertyDescriptor. With the preview build the same six throw hook-throw, and no other row of the 115 changes. The others throw hook-throw and do not abort: e.stack, Object.hasOwn, in, Object.getOwnPropertyNames, delete, assignment, Object.defineProperty, Object.freeze, structuredClone, postMessage. Their callers check for an exception right after the lookup (JSObject::getNonIndexPropertySlot does so, for example).

structuredClone(error) and postMessage(error) do not abort because Bun already works around this bug: SerializedScriptValue.cpp calls materializeErrorInfoIfNeeded() and checks for an exception before it asks for a descriptor.

Node 26.3 calls Error.prepareStackTrace only for a read of stack. It calls it 0 times for the six descriptor lookups above and for a delete, and its delete removes stack. Bun calls it once for each of them, because any first touch creates all four properties. That difference is older than this change, and this change does not try to remove it. With a throwing callback, Bun 1.4.3 throws hook-throw from delete e.stack and stack is gone afterwards. With this change it throws and stack stays, with the default string that Bun stores before it calls the callback.

Why the check is only made when this call created the properties: a lookup of any other name cannot throw, and isFetchError and maybeDuplicateFetchError rely on that. The check also leaves a lookup alone when it is made from inside the callback, where materializeErrorInfoIfNeeded() returns false.

getOwnSpecialPropertyNames needs no change. It returns void, and its caller JSObject::getOwnNonIndexPropertyNames checks for an exception right after it.

This branch starts at 000c489972, the commit that Bun's main pins today, so the preview build carries this change alone.

Related open PRs in the same file: #644 (integrity levels in materializeErrorInfoIfNeeded), #511, #535 (addErrorInfo under validateExceptionChecks=1, a different caller of the same callback). None of them touches these two functions. #644 edits the function right above, so the second of the two to land needs a rebase with no conflict expected.

[github-actions]

Preview build of 740d34a: autobuild-preview-pr-687-740d34a2

[claude]

I reviewed this PR and didn't find any bugs. Because it changes exception handling in a core JSC property hook (with one observable behavior change for delete e.stack under a throwing callback) and its tests live in the Bun repo rather than here, a human look would still be worthwhile.

What was reviewed:

• The exceptionForInspection()-without-ThrowScope check mirrors the existing pattern in Lookup.cpp setUpStaticFunctionSlot, and callers such as JSObject::getNonIndexPropertySlot do RETURN_IF_EXCEPTION right after the hook, so the false return propagates correctly.
• Checked the pre-existing-exception concern: the check only fires when this call ran the callback, and a lookup with an unrelated exception already pending would have been invalid before the callback ran, so nothing new is misattributed.
• Checked the fast path after a failed materialization: m_errorInfoMaterialized stays true, so later lookups skip the check and serve the stored stack/line/column directly; private-name lookups (isFetchError) never materialize and never hit the check.

Extended reasoning...

Overview

The PR is a single commit touching Source/JavaScriptCore/runtime/ErrorInstance.cpp (+12/-2). ErrorInstance::getOwnPropertySlot and ErrorInstance::deleteProperty now capture the return of materializeErrorInfoIfNeeded(vm, propertyName) and, when materialization ran this call and vm.exceptionForInspection() reports a pending exception, return false before touching the base implementation. The two siblings put and defineOwnProperty already had a DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION check; after this change all four property hooks refuse to proceed with a pending exception from the Bun-only VM::onComputeErrorInfoJSValue() callback.

Security risks

None specific to this change. It does not touch auth, crypto, or memory-unsafe operations; the only effect is returning "not found"/"not deleted" when an exception is already pending, which is the state JSC's own EXCEPTION_ASSERT(!scope.exception() || !result) contract in JSObject::getOwnPropertyDescriptor requires. Skipping Base::getOwnPropertySlot leaves the slot untouched, which is the correct state for a false return.

Level of scrutiny

Moderate. The diff is small and the reasoning is well documented, and the ThrowScope-free exceptionForInspection() pattern is already established in this fork (Lookup.cpp setUpStaticFunctionSlot, with the same justification about validateExceptionChecks). I verified that JSObject::getNonIndexPropertySlot checks for an exception immediately after calling an overriding getOwnPropertySlot, so the false return propagates correctly rather than continuing up the prototype chain. The three candidate issues from the hunt (misattributing an unrelated pending exception, the changed delete result, and the worker-termination case) are all either pre-existing invalid states or intentional consistency with put/defineOwnProperty, as the description states. Still, it is a core engine hook with subtle exception-scope semantics and a user-visible change to delete error.stack when Error.prepareStackTrace throws, so a maintainer's nod on the design choice is worthwhile.

Other factors

No tests are added in this repository, and none can be: the jsc shell cannot install the Bun callback, so verification is in oven-sh/bun#43096 against the preview build, which this review cannot run. The bug hunt ran dry (dry_streak) with no findings. No CODEOWNERS file constrains the path. The PR notes potential rebase against #644, which edits the adjacent function but not these two hooks.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 12b301fe-fd19-4426-9055-9caf1fbd686d

📥 Commits

Reviewing files that changed from the base of the PR and between 000c489 and 740d34a.

📒 Files selected for processing (1)

• Source/JavaScriptCore/runtime/ErrorInstance.cpp

Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review.

---

Walkthrough

ErrorInstance now stops property lookup and deletion when lazy error-property materialization leaves an exception pending. Successful deletion preserves cache disabling before delegating to the base implementation.

Changes

Error property operation handling

Layer / File(s)	Summary
Materialization exception handling Source/JavaScriptCore/runtime/ErrorInstance.cpp	getOwnPropertySlot and deleteProperty return false when materialization throws. Successful deletion still disables caching before base deletion.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 740d3

The change addresses pending exceptions during lazy Error property operations, and no current production or workflow risk requiring a pre-merge fix is identified.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the affected ErrorInstance operations and the behavior change when the error-info callback throws.
Description check	✅ Passed	The description is detailed, relevant, and explains the problem, fix, behavior change, implementation constraints, and verification. It does not include the template's Bugzilla link or Reviewed by lin…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.

---

_{Comment @coderabbitai help to get the list of available commands.}