Defer termination at the reifyStaticProperty call sites and propagate builder exceptions - #282
Conversation
LazyPropertyCallback runs inside reifyStaticProperty -> setUpStaticFunctionSlot -> getOwnPropertySlot, none of which check for exceptions after the builder. When worker.terminate() arms the termination trap while a builder enters JS, the trap fires inside the builder and the TerminationException stays pending after it returns; getOwnPropertySlot then returns true with an exception pending and trips EXCEPTION_ASSERT(!scope.exception() || !hasSlot) in JSValue::get / JSObject::getOwnPropertyDescriptor. LazyProperty::callFunc already wraps its initializer in DeferTerminationForAWhile for the same reason. Do the same for the PropertyCallback arm of reifyStaticProperty, covering both callers (setUpStaticFunctionSlot from getOwnPropertySlot and reifyAllStaticProperties from deleteProperty).
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesStatic property reification exception handling
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![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 — the change is small and mirrors the existing DeferTerminationForAWhile guard in LazyProperty::callFunc — but it sits in the core getOwnPropertySlot reification path and adjusts termination semantics, so it's worth a human sign-off.
What was reviewed:
- Confirmed
DeferTerminationForAWhileis the alias inDeferTermination.hand the include resolves. - Verified
LazyPropertyInlines.h:102uses the identical pattern the PR cites as precedent. - Checked scope placement — the guard is local to the
PropertyCallbackarm only, other reify branches are untouched.
Extended reasoning...
Overview
The PR adds a DeferTerminationForAWhile RAII scope around the PropertyAttribute::PropertyCallback branch of reifyStaticProperty in Lookup.h, plus the corresponding #include "DeferTermination.h". The intent is to prevent a TerminationException (armed by worker.terminate()) from firing inside a lazy-property builder that enters JS, which would otherwise leave an exception pending when getOwnPropertySlot returns true and trip EXCEPTION_ASSERT(!scope.exception() || !hasSlot).
Security risks
None identified. This does not touch auth, parsing, or memory layout. The only behavioural effect is that VM termination is deferred for the duration of the builder callback — the same trade-off already accepted for LazyProperty::callFunc.
Level of scrutiny
Medium-high. The diff is five lines, but reifyStaticProperty sits on the hot getOwnPropertySlot path for every object with a static hashtable, and it changes when a worker's termination trap can fire. The pattern is precedented (LazyPropertyInlines.h wraps its initializer identically), which lowers risk considerably, but a JSC maintainer should confirm this is the preferred fix vs. making the callers exception-aware, and that no in-tree LazyPropertyCallback can run long enough to make termination feel unresponsive.
Other factors
- The
DeferTermination.hheader is self-contained and already included transitively in many runtime TUs, so the new include is low-risk for build breakage. - The PR description includes a deterministic repro and references a companion Bun-side interim fix (oven-sh/bun#33966), which gives good context.
- No prior human review comments on the PR; only a rate-limited coderabbit stub.
Preview Builds
|
Per review: keep the include out of Lookup.h. DeferTerminationForAWhile now wraps the reifyStaticProperty call in setUpStaticFunctionSlot (Lookup.cpp) and the reify loop in JSObject::reifyAllStaticProperties. Also propagate a non-termination exception from a PropertyCallback builder: reifyStaticProperty skips the putDirect when the callback returns empty, setUpStaticFunctionSlot returns false (slot not found) when an exception is pending so JSValue::get / getOwnPropertyDescriptor's EXCEPTION_ASSERT(!scope.exception() || !result) holds, and reifyAllStaticProperties leaves the remaining properties lazy.
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/runtime/JSObject.cpp`:
- Around line 2963-2968: In JSObject::deleteProperty, add an immediate
vm.exceptionForInspection() check after reifyAllStaticProperties() returns, and
return without further reading or mutating structure when an exception is
pending. Preserve the existing delete behavior when reification completes
without throwing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0073c0fc-f836-4b6d-b784-12defd71ed99
📒 Files selected for processing (3)
Source/JavaScriptCore/runtime/JSObject.cppSource/JavaScriptCore/runtime/Lookup.cppSource/JavaScriptCore/runtime/Lookup.h
There was a problem hiding this comment.
I didn't find any bugs, but this touches core JSC property-lookup and termination/exception semantics, so it's worth a human look.
Checked that DeferTerminationForAWhile is already available in JSObject.cpp (used elsewhere in the file, so no missing include).
Checked that the new return false in setUpStaticFunctionSlot keeps the EXCEPTION_ASSERT(!scope.exception() || !result) invariant for its getStaticPropertySlotFromTable caller.
Checked that reifyAllStaticProperties callers (JSObject::deleteProperty, ObjectConstructor.cpp, JSGlobalObjectFunctions.cpp) all sit under scopes that will observe the propagated exception.
Noted the third reifyStaticProperty call site — the reifyStaticProperties template in Lookup.h — is intentionally left un-deferred; worth a human confirming that path never hits a JS-entering PropertyCallback.
Extended reasoning...
Overview
This PR wraps the two reifyStaticProperty call sites that can invoke a PropertyCallback builder (setUpStaticFunctionSlot in Lookup.cpp and JSObject::reifyAllStaticProperties in JSObject.cpp) in DeferTerminationForAWhile, and teaches both to bail out when an exception is pending afterwards. It also makes reifyStaticProperty's PropertyCallback arm skip putDirect when the callback returns an empty JSValue. The goal is to stop a TerminationException armed by worker.terminate() from surfacing mid-reify and tripping EXCEPTION_ASSERT(!scope.exception() || !hasSlot) in JSValue::get / getOwnPropertyDescriptor.
Security risks
None identified. This is exception-propagation plumbing; no new inputs, parsing, auth, or memory layout changes. The if (!result) return; guard in Lookup.h is strictly defensive — it prevents an empty JSValue from being stored via putDirect, which would otherwise be the more dangerous outcome.
Level of scrutiny
High. setUpStaticFunctionSlot sits on the getOwnPropertySlot path — one of the hottest and most invariant-laden paths in the VM — and reifyAllStaticProperties changes observable state (which properties are materialized vs. still lazy, whether staticPropertiesReified gets set). The change is small and mirrors the existing LazyProperty::callFunc precedent, but the interaction between DeferTerminationForAWhile, vm.exceptionForInspection(), and callers' ThrowScope/CatchScope conventions is subtle enough that a JSC-familiar reviewer should confirm the contract.
Other factors
- The PR description is detailed with a deterministic repro, and the pattern directly mirrors
LazyProperty::callFunc(LazyPropertyInlines.h:102), which is reassuring. JSObject.cppalready usesDeferTerminationForAWhilewithout a direct#include "DeferTermination.h"(lines ~3052/3317/3462), so the new use compiles via the existing transitive include.- The template
reifyStaticPropertiesinLookup.his a third caller ofreifyStaticPropertyand is not wrapped; the PR comment names only two callers. That's likely intentional (it runs atfinishCreationtime, not from JS-triggered lookup), but it's the kind of judgment call a human reviewer should sign off on. - Early-returning from
reifyAllStaticPropertiesleaves the object partially reified withstaticPropertiesReifiedstill false — consistent, but again worth a human confirming that's the intended semantics for the delete/assign/freeze callers. - No prior human review on the thread; only bot comments.
…sites exceptionForInspection() is for assertions/diagnostics only (every other use in JSC is inside ASSERT or #if ASSERT_ENABLED). It bypasses exception scope verification because it doesn't clear m_needExceptionCheck. Declare a ThrowScope and use RETURN_IF_EXCEPTION, matching the pattern at JSGlobalObject.cpp tryInstallSpeciesWatchpoint / JSBoundFunction.cpp nameSlow. In setUpStaticFunctionSlot the defer scope ends before the check so a suspended termination that is re-thrown on defer exit is observed and we return false, satisfying the caller's EXCEPTION_ASSERT(!scope.exception() || !hasSlot).
|
Replaced
In |
There was a problem hiding this comment.
No bugs found, but this touches core JSC property-lookup and termination/exception-scope semantics on a hot path, so it's worth a human look before merging.
What was reviewed:
- Confirmed the defer+ThrowScope pattern matches
LazyProperty::callFuncandJSBoundFunction::nameSlow. - Checked that
getOwnStaticPropertySlot's classInfo walk continuing after afalsereturn with a pending exception still yieldsfalseoverall, so the caller'sEXCEPTION_ASSERTholds. - Verified the empty-result guard in
reifyStaticPropertyonly affects thePropertyCallbackarm and skipsputDirectrather than storing an empty JSValue.
Extended reasoning...
Overview
The PR adds DeferTerminationForAWhile + ThrowScope/RETURN_IF_EXCEPTION around the two call sites of reifyStaticProperty that can invoke a PropertyCallback builder (setUpStaticFunctionSlot in Lookup.cpp and JSObject::reifyAllStaticProperties in JSObject.cpp), and adds an empty-result guard in reifyStaticProperty's PropertyCallback arm in Lookup.h. The goal is to fix a debug assertion (!scope.exception() || !hasSlot) that fires when worker.terminate() arms the termination trap while a builder is running JS.
Security risks
None identified. This is exception-propagation plumbing; it does not change access control, parsing, or expose new surface. The empty-value guard prevents an empty JSValue from being stored via putDirect, which is strictly safer than before.
Level of scrutiny
High. setUpStaticFunctionSlot sits directly in the getOwnPropertySlot path — one of the hottest and most correctness-sensitive paths in the VM. Introducing a ThrowScope and a new early-return branch there, plus a DeferTerminationForAWhile in reifyAllStaticProperties, changes exception-scope-verification behavior and control flow in ways that are subtle to reason about (defer scope destructor ordering vs. throw-scope check, DeferForAWhile re-arming as a trap vs. re-throwing, interaction with callers that lack a ThrowScope like deleteProperty). The author's reasoning and the cited precedents look correct, but this is not a mechanical change I'm comfortable auto-approving.
Other factors
- The PR went through an iteration: an earlier revision used
vm.exceptionForInspection()and was corrected to a properThrowScope— the right call, but it means the current shape hasn't yet been reviewed by a human. - CodeRabbit's
deletePropertyconcern was discussed and withdrawn as pre-existing; that thread is resolved. - Bug hunters raised and refuted a concern about the classInfo walk continuing after
setUpStaticFunctionSlotreturnsfalsewith an exception; I re-checkedgetOwnStaticPropertySlotand agree the final result is stillfalse, so the caller assertion holds. - Preview builds exist for the earlier commits but not yet for e9de788 (the ThrowScope revision).
…ffset #282 added a ThrowScope here so that a PropertyCallback builder exception (or a re-thrown termination after the defer scope ends) makes setUpStaticFunctionSlot return false. But a ThrowScope's destructor always simulates a throw to its caller, so every getOwnStaticPropertySlot / getOwnNonIndexPropertySlot / getPropertySlot call site on the property-lookup fast path would need a RETURN_IF_EXCEPTION. Under validateExceptionChecks=1 the second DECLARE_THROW_SCOPE at the same depth (next table in getOwnStaticPropertySlot's loop, or the next property lookup) asserts. reifyStaticProperty already skips the putDirect when the callback returned empty, so getDirectOffset stays invalid in exactly the cases the ThrowScope was detecting. Return false on an invalid offset with an ASSERT(vm.exceptionForInspection()) instead of forcing a viral exception check onto the fast path.
… builder exceptions (#282) * Defer termination across static-hashtable PropertyCallback builders LazyPropertyCallback runs inside reifyStaticProperty -> setUpStaticFunctionSlot -> getOwnPropertySlot, none of which check for exceptions after the builder. When worker.terminate() arms the termination trap while a builder enters JS, the trap fires inside the builder and the TerminationException stays pending after it returns; getOwnPropertySlot then returns true with an exception pending and trips EXCEPTION_ASSERT(!scope.exception() || !hasSlot) in JSValue::get / JSObject::getOwnPropertyDescriptor. LazyProperty::callFunc already wraps its initializer in DeferTerminationForAWhile for the same reason. Do the same for the PropertyCallback arm of reifyStaticProperty, covering both callers (setUpStaticFunctionSlot from getOwnPropertySlot and reifyAllStaticProperties from deleteProperty). * Move the defer to the call sites and propagate exceptions Per review: keep the include out of Lookup.h. DeferTerminationForAWhile now wraps the reifyStaticProperty call in setUpStaticFunctionSlot (Lookup.cpp) and the reify loop in JSObject::reifyAllStaticProperties. Also propagate a non-termination exception from a PropertyCallback builder: reifyStaticProperty skips the putDirect when the callback returns empty, setUpStaticFunctionSlot returns false (slot not found) when an exception is pending so JSValue::get / getOwnPropertyDescriptor's EXCEPTION_ASSERT(!scope.exception() || !result) holds, and reifyAllStaticProperties leaves the remaining properties lazy. * Use a ThrowScope instead of exceptionForInspection at the reify call sites exceptionForInspection() is for assertions/diagnostics only (every other use in JSC is inside ASSERT or #if ASSERT_ENABLED). It bypasses exception scope verification because it doesn't clear m_needExceptionCheck. Declare a ThrowScope and use RETURN_IF_EXCEPTION, matching the pattern at JSGlobalObject.cpp tryInstallSpeciesWatchpoint / JSBoundFunction.cpp nameSlow. In setUpStaticFunctionSlot the defer scope ends before the check so a suspended termination that is re-thrown on defer exit is observed and we return false, satisfying the caller's EXCEPTION_ASSERT(!scope.exception() || !hasSlot).
…nstructVersions to RETURN_IF_EXCEPTION setUpStaticFunctionSlot now wraps reifyStaticProperty in DeferTerminationForAWhile and treats a pending exception as slot-not-found (oven-sh/WebKit#282, #306, landed in bun via #34373). reifyStaticProperty itself early-returns on an empty callback result instead of putDirect'ing it. So PropertyCallback builders can RETURN_IF_EXCEPTION(scope, {}) and the exception propagates to JS correctly. This removes the builder-level DeferTerminationForAWhile this PR had added to constructStdioWriteStream/constructStdin/constructEnv/constructNextTickFn and callLazyProcessBuilder (now redundant with the JSC-side wrapping), and reverts constructVersions to main's RETURN_IF_EXCEPTION. The pre-existing clear-and-report patterns on main are left alone.
2 participants
LazyPropertyCallbackruns insidereifyStaticProperty→setUpStaticFunctionSlot→getOwnPropertySlot, none of which check for exceptions after the builder. Whenworker.terminate()arms the termination trap while a builder enters JS, the trap fires inside the builder and theTerminationExceptionstays pending after it returns;getOwnPropertySlotthen returnstruewith an exception pending and tripsEXCEPTION_ASSERT(!scope.exception() || !hasSlot)inJSValue::get/JSObject::getOwnPropertyDescriptor.LazyProperty::callFuncalready wraps its initializer inDeferTerminationForAWhilefor the same reason. This does the same at the two call sites ofreifyStaticPropertythat can hit aPropertyCallbackbuilder (kept out ofLookup.hto avoid pullingDeferTermination.hinto the header):setUpStaticFunctionSlot(Lookup.cpp): defer across the reify; afterwards, if an exception is pending, returnfalsesogetOwnPropertySlotreports the slot as not found and the caller'sEXCEPTION_ASSERT(!scope.exception() || !result)holds.JSObject::reifyAllStaticProperties: defer across the loop; on exception, bail without settingstaticPropertiesReified(true)so the remaining properties stay lazy and the caller propagates.reifyStaticProperty'sPropertyCallbackarm also skipsputDirectwhen the callback returns an empty value, so an empty JSValue never lands in the slot.Bun hits this via its
process.stdout/stderr/stdin/nextTickbuilders, which enter JS to loadnode:stream: oven-sh/bun#33966 adds per-builderDeferTerminationForAWhilescopes as an interim fix that become redundant once this lands and the WebKit version is bumped. Deterministic repro on a debug build:(aborts with the
!scope.exception() || !hasSlotassertion on every run before the fix.)