JSC: tolerate pending TerminationException in property-lookup assertions#286
JSC: tolerate pending TerminationException in property-lookup assertions#286Jarred-Sumner wants to merge 2 commits into
Conversation
DeferTermination throws the TerminationException from its destructor, after a property-slot lookup has already succeeded. JSObject::get already tolerates this (hasPendingTerminationException in its assertion); three sibling sites did not and abort assertion builds when terminate() races script execution: - JSValue::get (both overloads): same tolerance + RETURN_IF_EXCEPTION, hit via LLInt get_by_id during module evaluation - JSObject::getOwnPropertyDescriptor: same, hit via Object.defineProperty - JSModuleLoader::continueDynamicImport: assertNoException -> assertNoExceptionExceptTermination (rejectWithCaughtException cannot clear a termination, so it stays pending by design) No exception is ever cleared; callers' existing checks unwind as usual. Reproduced with a worker terminate() stress loop: 3/320 aborts before, 0/320 + 0/240 after.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesTermination exception handling
Suggested reviewers: 🚥 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 mirrors the existing JSObject::get pattern in JSObjectInlines.h:137-138 exactly, and assertNoExceptionExceptTermination() is the standard idiom for this. Deferring only because JSValue::get and JSObject::getOwnPropertyDescriptor are core hot paths and the added RETURN_IF_EXCEPTION is a (benign-looking) release-build change; a JSC maintainer should give it a quick glance.
What was reviewed:
- Confirmed the precedent:
JSObjectInlines.halready usesEXCEPTION_ASSERT(... || vm.hasPendingTerminationException() || ...)+RETURN_IF_EXCEPTIONfor bothJSObject::getoverloads. EXCEPTION_ASSERT_UNUSED→EXCEPTION_ASSERTingetOwnPropertyDescriptoris correct sincescopeis now used byRETURN_IF_EXCEPTION.- The new
RETURN_IF_EXCEPTIONdoesn't change the non-termination path (invariant already guaranteed!hasSlot→jsUndefined()); only the termination-race case now returns early instead of proceeding intoslot.getValue().
Extended reasoning...
Overview
This PR relaxes four debug-only exception assertions to tolerate a pending TerminationException, matching the established pattern already present in JSObject::get (JSObjectInlines.h:137,153). The affected sites are:
JSValue::get(JSGlobalObject*, PropertyName, PropertySlot&)and theunsignedoverload inJSCJSValuePropertyInlines.hJSObject::getOwnPropertyDescriptorinJSObject.cppJSModuleLoader::continueDynamicImportinJSModuleLoader.cpp(assertNoException→assertNoExceptionExceptTermination)
Each site also gains a RETURN_IF_EXCEPTION so that when the race does occur, the function unwinds cleanly rather than calling into slot.getValue() with a pending exception.
Security risks
None. The change only weakens debug assertions to admit a case that already occurs at runtime (termination racing a successful slot lookup), and adds an early-return-on-exception which is strictly safer than proceeding. No exceptions are cleared or swallowed — RETURN_IF_EXCEPTION propagates and the sticky termination remains pending for the caller.
Level of scrutiny
Medium-high. The change itself is mechanical and precedented — I verified JSObjectInlines.h:137-138 and :153-154 use the identical construct, and assertNoExceptionExceptTermination() is used in 6 other runtime files. However, JSValue::get is one of the most central C++ property-access helpers in JSC, and getOwnPropertyDescriptor backs Object.defineProperty/Object.getOwnPropertyDescriptor. Adding a RETURN_IF_EXCEPTION to JSValue::get is a release-build change on a very widely-called path. The change should be a no-op for the non-exception case (one predicted-not-taken branch) and for ordinary exceptions (the old invariant guaranteed !hasSlot, so both old and new code return jsUndefined()), but a JSC maintainer should confirm.
Other factors
- The
EXCEPTION_ASSERT_UNUSED→EXCEPTION_ASSERTswitch ingetOwnPropertyDescriptoris correct:scopeis now used by the newRETURN_IF_EXCEPTION, so the_UNUSEDmarker is no longer needed. - The second
JSValue::getoverload callsgetVM(globalObject)inline rather than caching it in a local like the first overload; harmless (getVMis a trivial pointer chase) but slightly inconsistent. - The PR description notes other
EXCEPTION_ASSERT(!scope.exception() || !success)sites (ProxyObject, JSONObject) are latently susceptible; a maintainer may want to weigh in on whether those should be batched in. - Empirically verified per the description (320-run stress loop, 3→0 aborts).
- No prior reviews or comments on the PR.
There was a problem hiding this comment.
LGTM — mechanical application of the existing JSObject::get termination-tolerance pattern to three sibling sites.
What was reviewed:
- Confirmed
JSObjectInlines.h:137/153already uses the identicalhasPendingTerminationException()+RETURN_IF_EXCEPTIONshape being copied here. assertNoExceptionExceptTermination()is an existing helper (ExceptionScope.h:63/101); the module-loader change is a straight swap.EXCEPTION_ASSERT_UNUSED→EXCEPTION_ASSERTingetOwnPropertyDescriptoris correct now thatscopeis used by the newRETURN_IF_EXCEPTION.- The added early returns only fire with an exception already pending, so callers' own
RETURN_IF_EXCEPTIONchecks discard the sentinel — no observable behavior change in the non-termination path.
Extended reasoning...
Overview
The PR relaxes four debug-only assertion sites to tolerate a pending TerminationException and adds RETURN_IF_EXCEPTION early-outs at three of them. Touches JSValue::get (two overloads), JSObject::getOwnPropertyDescriptor, and one assertNoException() → assertNoExceptionExceptTermination() swap in JSModuleLoader::continueDynamicImport. The motivation is a worker.terminate() race where DeferTermination's destructor throws after a slot lookup already succeeded, tripping !scope.exception() || !result in assertion builds.
Security risks
None. The change loosens debug assertions by one specific disjunct (vm.hasPendingTerminationException()) and adds early returns that only trigger when an exception is already set. No exception is cleared, no new state is exposed, and release-build semantics change only in that a pending-exception path now short-circuits rather than proceeding into slot.getValue() / descriptor.setPropertySlot() — which is strictly more defensive.
Level of scrutiny
Low-to-medium. Although JSValue::get is an extremely hot inlined path, this is a verbatim copy of the pattern already present in JSObject::get (JSObjectInlines.h:137, :153) — same assertion shape, same RETURN_IF_EXCEPTION(scope, jsUndefined()). The EXCEPTION_ASSERT compiles out entirely in release; the only release-visible addition is the RETURN_IF_EXCEPTION branch, which is standard JSC exception hygiene and mirrors the sibling function exactly. The getOwnPropertyDescriptor change correctly drops the _UNUSED macro variant now that scope is consumed by RETURN_IF_EXCEPTION. The module-loader change uses an existing purpose-built helper.
Other factors
The PR description documents empirical verification: 3/320 → 0/320 aborts under a terminate() stress loop with validateExceptionChecks=1, plus 0/240 on the original failing test. The fix is narrowly scoped to the three empirically observed abort sites rather than a blanket sweep, and explicitly notes other latently-susceptible sites are left for later — a conservative approach. No design decisions are being made here; this is CI-stability plumbing following upstream precedent.
Preview Builds
|
The previous preview was based on WebKit main, which pulled in the JSMicrotask/JSPromise refactors (oven-sh/WebKit#295/#301) and nudged the node-net mimalloc-page-count leak test past its threshold on aarch64. The minimal branch carries only the termination fixes the new stress test needs (plus Jarred's oven-sh/WebKit#286 for the property-lookup assertions the same sweep surfaces).
1 participant
Assertion builds abort when
worker.terminate()races script execution:DeferTerminationthrows the TerminationException from its destructor, i.e. after a property-slot lookup already succeeded, trippingASSERTION FAILED: !scope.exception() || !result.JSObject::getalready tolerates this viahasPendingTerminationException()in its assertion (upstream pattern). This applies the same tolerance to the three sibling sites observed aborting in CI stress runs:JSValue::get(both overloads in JSCJSValuePropertyInlines.h) — hit via LLIntget_by_idduring module evaluationJSObject::getOwnPropertyDescriptor(JSObject.cpp:3936) — hit viaObject.definePropertyJSModuleLoader::continueDynamicImport—assertNoException()→assertNoExceptionExceptTermination()(rejectWithCaughtException'sTRY_CLEAR_EXCEPTIONcannot clear a termination, so it stays pending by design)No exception is ever cleared — only the sticky termination is tolerated and
RETURN_IF_EXCEPTIONlets callers unwind.Verification: worker
terminate()stress loop (320 runs under load,BUN_JSC_validateExceptionChecks=1): 3/320 aborts before, 0/320 after, plus 0/240 on the original failing node test. OtherEXCEPTION_ASSERT(!scope.exception() || !success)sites (ProxyObject, JSONObject, …) are latently susceptible to the same race; this fixes the three empirically observed, matching theJSObject::getprecedent.