Drop the ThrowScope from the reifyStaticProperty call sites - #306
Conversation
ae5110d added DECLARE_THROW_SCOPE inside setUpStaticFunctionSlot and reifyAllStaticProperties so a PropertyCallback builder that throws can be detected and propagated. A ThrowScope's destructor unconditionally simulates a throw to its caller, so this made both functions look throwing to the exception-scope verifier. setUpStaticFunctionSlot runs on every first lookup of a static hashtable property via getOwnPropertySlot, and JSC-internal callers of that path (CodeBlock::finishCreation via initializeTemplateObjects, among others) never check afterward, so with validateExceptionChecks=1 they now crash even though nothing threw. reifyAllStaticProperties has the same issue for JSObject::deleteProperty. Keep the DeferTerminationForAWhile and the early-return-on-exception, but read vm.exceptionForInspection() directly instead of declaring a ThrowScope. That preserves the ae5110d behaviour (report the slot as not found when the builder left an exception pending, bail from the reify-all loop) without forcing a simulated throw onto callers that have never had to handle one.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues, but this touches JSC's exception-scope machinery on the core property-lookup path, so it's worth a human sign-off.
Checked that vm.exceptionForInspection() reads m_exception directly (VM.h:840) — matches the intent of detecting a builder-thrown exception without the ThrowScope destructor's simulated throw. Confirmed LazyProperty::callFunc (the cited precedent) uses DeferTerminationForAWhile with no ThrowScope. In reifyAllStaticProperties the defer scope is still active at the check, and in setUpStaticFunctionSlot the ForAWhile variant doesn't re-throw on destruction, so reading m_exception directly is equivalent to the prior RETURN_IF_EXCEPTION for the non-termination case.
Extended reasoning...
Overview
This PR adjusts two call sites that #282 recently modified: setUpStaticFunctionSlot in Lookup.cpp and JSObject::reifyAllStaticProperties in JSObject.cpp. #282 had added DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION so that a PropertyCallback builder throwing during lazy reification would be detected. The problem is that a ThrowScope's destructor unconditionally simulates a throw to its caller under validateExceptionChecks=1, and these functions sit under getOwnPropertySlot / deleteProperty — paths whose many internal callers have never needed to check for an exception. The fix keeps the DeferTerminationForAWhile guard and the early-return-on-exception, but reads vm.exceptionForInspection() directly instead of declaring a ThrowScope.
Security risks
None. This is purely about the exception-scope verifier's bookkeeping and early-return control flow; no new data flows, no auth/crypto/permissions surface.
Level of scrutiny
High. Both sites are on critical JSC runtime paths — setUpStaticFunctionSlot runs on every first lookup of a static hashtable property, and reifyAllStaticProperties is reached from JSObject::deleteProperty. The semantics of RETURN_IF_EXCEPTION (which routes through vm.traps().maybeNeedHandling() / hasExceptionsAfterHandlingTraps()) vs. a raw m_exception read differ subtly around trap handling. I convinced myself the difference is benign here (termination is deferred, and the ForAWhile variant re-arms the trap rather than re-throwing on destruction, so only a real builder exception can be in m_exception at the check), but JSC's exception-scope verifier is subtle enough that a maintainer familiar with it should confirm.
Other factors
The PR description is thorough: it includes a concrete repro (BUN_JSC_validateExceptionChecks=1 on the bun test suite), a before/after table across five affected tests, and confirmation that #282's original worker-terminate repro still passes. The cited precedent (LazyProperty::callFunc) checks out — it uses DeferTerminationForAWhile with no ThrowScope. exceptionForInspection() is documented in VM.h as intended for exactly this use ("check for any pending exception without interfering with Throw/CatchScopes"). The bug-hunting pass found nothing. I'm deferring solely because this is core runtime exception-handling code, not because I see a problem.
Preview Builds
|
ae5110d added DECLARE_THROW_SCOPE inside setUpStaticFunctionSlot and reifyAllStaticProperties so a PropertyCallback builder that throws can be detected and propagated. A ThrowScope's destructor unconditionally simulates a throw to its caller, so this made both functions look throwing to the exception-scope verifier. setUpStaticFunctionSlot runs on every first lookup of a static hashtable property via getOwnPropertySlot, and JSC-internal callers of that path (CodeBlock::finishCreation via initializeTemplateObjects, among others) never check afterward, so with validateExceptionChecks=1 they now crash even though nothing threw. reifyAllStaticProperties has the same issue for JSObject::deleteProperty. Keep the DeferTerminationForAWhile and the early-return-on-exception, but read vm.exceptionForInspection() directly instead of declaring a ThrowScope. That preserves the ae5110d behaviour (report the slot as not found when the builder left an exception pending, bail from the reify-all loop) without forcing a simulated throw onto callers that have never had to handle one.
Resolves the Lookup.cpp conflict by taking main's #306 version of setUpStaticFunctionSlot: both branches independently dropped the ThrowScope from the reifyStaticProperty call site; main's version checks vm.exceptionForInspection() directly and keeps the original RELEASE_ASSERT_NOT_REACHED for the no-exception-but-no-offset case. Also brings in #304 (Windows WallTime/MonotonicTime) and #310 (preCommitStackMemory guard).
2 participants
ae5110d (#282) added
DECLARE_THROW_SCOPEinsidesetUpStaticFunctionSlotandreifyAllStaticPropertiesso a PropertyCallback builder that throws can be detected and propagated. A ThrowScope's destructor unconditionally simulates a throw to its caller, so this made both functions look throwing to the exception-scope verifier.setUpStaticFunctionSlotruns on every first lookup of a static hashtable property (viagetStaticPropertySlotFromTable→getOwnPropertySlot), and JSC-internal callers of that path never check afterward. WithvalidateExceptionChecks=1they crash even though nothing threw:reifyAllStaticPropertieshas the same issue forJSObject::deleteProperty, which reaches it without a scope.This keeps the
DeferTerminationForAWhileand the early-return-on-exception, but readsvm.exceptionForInspection()directly instead of declaring a ThrowScope. That preserves #282's behaviour (report the slot as not found when the builder left an exception pending, bail from the reify-all loop) without forcing a simulated throw onto callers that have never had to handle one.LazyProperty::callFunc, the original model for #282's defer-termination, does not declare a scope either.Repro
Build bun with
WEBKIT_VERSIONat any post-#282 commit (ae5110d3071for later), then on a debug/ASAN build:BUN_JSC_validateExceptionChecks=1 bun-debug test test/js/bun/globals.test.jsfails at the
globals are deletabletest (and ~20 other tests in oven-sh/bun#34428's CI run https://buildkite.com/bun/bun/builds/74528 debian-13 x64-asan lane).Verification
Built bun locally against this branch (debug+ASAN,
validateExceptionChecks=1):The #282 worker-terminate repro (terminate while
process.stdoutis being lazily built) still passes.