JSModuleLoader: don't treat a TerminationException from resolve() as a resolution failure - #309
JSModuleLoader: don't treat a TerminationException from resolve() as a resolution failure#309robobun wants to merge 4 commits into
Conversation
WalkthroughJavaScriptCore now propagates termination during property lookup and static-property reification. Module resolution bypasses ordinary failure handling for termination exceptions, and dynamic import rejection permits termination to remain pending. ChangesTermination-aware property reification and access
Module loader termination handling
Possibly related PRs
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.
LGTM — targeted fix for a debug-assert abort when a TerminationException surfaces during module resolution.
What was reviewed:
- Confirmed both callers of
hostLoadImportedModule(loadModuleandinnerModuleLoading) checkscope.exception()immediately after, so the newreturn nullptrunwinds correctly without leaking state. vm.isTerminationException(...)andassertNoExceptionExceptTermination()are both established patterns (used inCyclicModuleRecord.cpp,JSModuleNamespaceObject.cpp, etc.).- The early return correctly skips
addResolutionFailure, so the termination is not cached as a permanent resolution error.
Extended reasoning...
Overview
This PR touches a single file, Source/JavaScriptCore/runtime/JSModuleLoader.cpp, with two small changes: (1) in hostLoadImportedModule, when resolve() throws and the exception is a TerminationException, return nullptr immediately instead of treating it as a resolution failure (caching it and calling finishLoadingImportedModule with the termination still pending); and (2) in continueDynamicImport, relax scope.assertNoException() to scope.assertNoExceptionExceptTermination() after promise->reject(...).
Security risks
None. This is exception-handling control flow in the module loader for the worker-termination race. No new inputs, no auth/crypto/permission surfaces. The only behavioral difference in release builds is that a TerminationException no longer gets stashed in m_resolutionFailures — which is strictly more correct, since a termination is not a per-specifier resolution error that should be cached.
Level of scrutiny
Low-to-medium. The observable bug is a debug-only assertNoException() abort under ENABLE(EXCEPTION_SCOPE_VERIFICATION); release builds already unwind normally via the next RETURN_IF_EXCEPTION. The fix is 5 new lines plus a one-token assertion relaxation, both following idioms already present in this codebase (vm.isTerminationException(...) guards in ExceptionScope.h, assertNoExceptionExceptTermination() in CyclicModuleRecord.cpp / JSModuleNamespaceObject.cpp).
Other factors
I verified the PR description's claim that both callers handle the nullptr return: loadModule does RETURN_IF_EXCEPTION(scope, nullptr) on the next line, and innerModuleLoading does if (scope.exception()) { state->setDrainingInnerLoad(false); return; }. Since the TerminationException remains pending on the VM (it is checked, not cleared), both paths bail correctly. The early return also happens before addResolutionFailure, so nothing leaks into m_resolutionFailures. No prior reviews or outstanding comments on the PR.
Preview Builds
|
… 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).
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.
9cb2817 to
bd5d12b
Compare
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.
…a resolution failure When a worker is terminated while hostLoadImportedModule is inside resolve(), the NeedTermination trap surfaces as a TerminationException. Returning early lets the caller's RETURN_IF_EXCEPTION unwind instead of caching the termination string in m_resolutionFailures and entering FinishLoadingImportedModule with the termination still pending on the VM.
bd5d12b to
5113fb4
Compare
![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/JSModuleLoader.cpp`:
- Around line 1031-1034: Update the comment immediately before
scope.assertNoExceptionExceptTermination() to describe the actual
promise->reject(...) path: promise rejection does not clear the ambient
TerminationException, while termination is returned before becoming a cached
resolution failure, and the caller’s exception check unwinds it. Remove the
inaccurate reference to rejectWithCaughtException.
🪄 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: 70358b8e-bb04-40cc-8f3f-2bae52565652
📒 Files selected for processing (5)
Source/JavaScriptCore/runtime/JSCJSValuePropertyInlines.hSource/JavaScriptCore/runtime/JSModuleLoader.cppSource/JavaScriptCore/runtime/JSObject.cppSource/JavaScriptCore/runtime/Lookup.cppSource/JavaScriptCore/runtime/Lookup.h
| // The abrupt completion may be a resolution failure caused by the | ||
| // TerminationException, which rejectWithCaughtException cannot clear; | ||
| // it stays pending in the VM and the caller's exception check unwinds. | ||
| scope.assertNoExceptionExceptTermination(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the comment to describe the actual rejection path.
Lines [1031]-[1034] call promise->reject(...), not rejectWithCaughtException(...). Additionally, termination is now returned before becoming a cached resolution failure. Clarify that promise rejection does not clear the ambient TerminationException; the caller’s exception check unwinds it.
Suggested wording
- // The abrupt completion may be a resolution failure caused by the
- // TerminationException, which rejectWithCaughtException cannot clear;
- // it stays pending in the VM and the caller's exception check unwinds.
+ // Rejecting the promise does not clear a pending TerminationException;
+ // it remains pending for the caller's exception check to unwind.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The abrupt completion may be a resolution failure caused by the | |
| // TerminationException, which rejectWithCaughtException cannot clear; | |
| // it stays pending in the VM and the caller's exception check unwinds. | |
| scope.assertNoExceptionExceptTermination(); | |
| // Rejecting the promise does not clear a pending TerminationException; | |
| // it remains pending for the caller's exception check to unwind. | |
| scope.assertNoExceptionExceptTermination(); |
🤖 Prompt for 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.
In `@Source/JavaScriptCore/runtime/JSModuleLoader.cpp` around lines 1031 - 1034,
Update the comment immediately before scope.assertNoExceptionExceptTermination()
to describe the actual promise->reject(...) path: promise rejection does not
clear the ambient TerminationException, while termination is returned before
becoming a cached resolution failure, and the caller’s exception check unwinds
it. Remove the inaccurate reference to rejectWithCaughtException.
| // The abrupt completion may be a resolution failure caused by the | ||
| // TerminationException, which rejectWithCaughtException cannot clear; | ||
| // it stays pending in the VM and the caller's exception check unwinds. | ||
| scope.assertNoExceptionExceptTermination(); |
There was a problem hiding this comment.
🟡 nit: This comment references rejectWithCaughtException (which is called upstream in hostLoadImportedModule, not here — this site calls promise->reject()), and it describes a scenario — "resolution failure caused by the TerminationException" — that this PR's own hostLoadImportedModule fix now prevents from reaching continueDynamicImport. The relaxed assertion is still correct (since promise->reject() above can itself trip the NeedTermination trap), but consider rewording the comment to say that instead.
Extended reasoning...
The new comment above scope.assertNoExceptionExceptTermination() in continueDynamicImport reads:
The abrupt completion may be a resolution failure caused by the TerminationException, which rejectWithCaughtException cannot clear; it stays pending in the VM and the caller's exception check unwinds.
There are two accuracy issues with this wording in the context of the combined diff.
1. Wrong function referenced at this site. Line 1029 calls promise->reject(vm, (*exception)->value()), not rejectWithCaughtException. The rejectWithCaughtException reference points at the upstream resolution-error branch in hostLoadImportedModule (JSModuleLoader.cpp:663). A reader looking at this comment in isolation has to know it's describing a call in a different function.
2. The described scenario is now unreachable. The comment's causal chain is: resolve() throws TerminationException → rejectWithCaughtException can't clear it → finishLoadingImportedModule is entered with it pending → continueDynamicImport receives an Exception* completion. But this same PR adds, at JSModuleLoader.cpp:652–656:
if (vm.isTerminationException(resolutionError)) [[unlikely]]
return nullptr;which returns before attachErrorInfo / addResolutionFailure / rejectWithCaughtException / finishLoadingImportedModule. So a TerminationException from resolve() can no longer be cached in m_resolutionFailures nor flow into continueDynamicImport as the completion. Tracing the other Exception* paths into continueDynamicImport — the cached-resolution-failure branch at line 640 and the fetch-error branch at line 691 — both pass stored error values, not a pending TerminationException.
Step-by-step (post-fix):
- Worker termination fires while
hostLoadImportedModuleis insideresolve(). resolve()returns withTerminationExceptionpending;scope.exception()yields it.- New guard:
vm.isTerminationException(resolutionError)→return nullptr. - Caller's
RETURN_IF_EXCEPTIONunwinds.finishLoadingImportedModule/continueDynamicImportare never reached.
So the scenario the comment describes cannot happen anymore.
Why the relaxed assertion is still correct. promise->reject(vm, ...) at line 1029 can enter JS (reject handlers / HostPromiseRejectionTracker), and the NeedTermination trap can fire during that call, leaving a TerminationException pending on return. assertNoExceptionExceptTermination() correctly tolerates that. The code change is right; only the comment's justification is stale.
Context. Per the PR description, this comment was cherry-picked from #286, which was written before the hostLoadImportedModule fix existed — so it was accurate in isolation but is stale in the combined diff.
Suggested fix. Reword to something like:
// promise->reject() may enter JS and trip the NeedTermination trap; the
// TerminationException stays pending and the caller's exception check unwinds.This is comment-wording only; no behavioral defect.
| // A callback that enters JS may return empty with an exception pending; | ||
| // the two callers (setUpStaticFunctionSlot / reifyAllStaticProperties) | ||
| // check and propagate, so don't put an empty value in the slot here. | ||
| if (!result) [[unlikely]] | ||
| return; |
There was a problem hiding this comment.
🟡 nit: the comment says "the two callers" but reifyStaticProperties<ArrayType> at the bottom of this header is a third caller of reifyStaticProperty that neither wraps in DeferTerminationForAWhile nor checks vm.exceptionForInspection() afterwards. It's only invoked from WebCore-generated bindings so it's dead in Bun's JSCOnly build, but consider saying "the two JSC-side callers" so the comment doesn't mislead someone reading the full tree.
Extended reasoning...
What the comment claims vs. what's in the file
The new comment at Lookup.h:541-543 reads:
the two callers (setUpStaticFunctionSlot / reifyAllStaticProperties) check and propagate
However, reifyStaticProperty has three direct callers, not two:
setUpStaticFunctionSlot(Lookup.cpp:66) — now wraps inDeferTerminationForAWhileand checksvm.exceptionForInspection()afterwards.JSObject::reifyAllStaticProperties(JSObject.cpp:2965) — same treatment.reifyStaticProperties<ArrayType>(Lookup.h:~569-580) — the template at the bottom of this same header, which loops over aHashTableValuearray and callsreifyStaticPropertyfor each entry with noDeferTerminationForAWhilescope and no post-call exception check.
Step-by-step: what would happen on the third path
If a caller reached reifyStaticProperties<ArrayType> and one entry had PropertyAttribute::PropertyCallback whose builder entered JS and returned empty with an exception pending:
reifyStaticPropertyis called for entry N; the callback returns an emptyJSValuewith an exception on the VM.- The new
if (!result) return;guard fires (a strict improvement — before this PR it would have crashed inputDirecton an empty value). - Control returns to the
forloop inreifyStaticProperties<ArrayType>, which does not inspectvm.exceptionForInspection(). - The loop proceeds to entry N+1 and calls
reifyStaticPropertyagain with an exception already pending on the VM — exactly the state the two other callers were hardened against.
Why this doesn't matter for Bun
grep confirms the reifyStaticProperties<ArrayType> template is only defined inside Source/JavaScriptCore — every invocation lives in Source/WebCore/bindings/scripts/test/JS/*.cpp finishCreation paths generated by the WebCore bindings generator. Bun builds with PORT=JSCOnly, which does not compile WebCore, so the third caller is dead code in this repository's build. There is no reachable behavioral bug here.
Why it's still worth a nit
The comment lives right next to the if (!result) guard and is meant to justify why returning early without a local exception check is safe. A reader looking at the whole header will see reifyStaticProperties<ArrayType> thirty lines below, notice it also calls reifyStaticProperty, and wonder why the comment doesn't mention it. If a JSC-side caller of the template were ever added, the comment would actively mislead them into thinking the exception is handled.
Suggested fix
Reword to something like:
the two JSC-side callers (setUpStaticFunctionSlot / reifyAllStaticProperties) check and propagate; the reifyStaticProperties<> template below is only reached from WebCore bindings.
or simply "the two exception-checking callers". No code change needed — the if (!result) guard is a strict improvement on all three paths.
|
Superseded by #391, which landed the same hostLoadImportedModule fix (propagate the TerminationException from resolve() instead of caching it as a resolution failure). Closing. |
2 participants
When a worker is terminated while
hostLoadImportedModuleis insideresolve(), theNeedTerminationtrap surfaces as aTerminationExceptionfromresolve(). The existing code handles it like any other resolution error: it caches the termination string inm_resolutionFailures, callspromise->rejectWithCaughtException(whoseTRY_CLEAR_EXCEPTIONrefuses to clear a termination and returns early), and then entersfinishLoadingImportedModulewith the termination still pending on the VM.Seen in Bun's debug+ASAN CI at roughly 1 in 100 runs of
new Worker("", { eval: true })followed immediately byworker.terminate(). A calibrated sweep (terminating workers across the tail of their startup preload) surfaces three assertions on the unfixed build:scope.assertNoException()incontinueDynamicImport(JSModuleLoader.cpp)JSObject::putDirectviareifyStaticProperty(Lookup.h)EXCEPTION_ASSERT(!scope.exception() || !result)inJSObject::getOwnPropertyDescriptorFix
hostLoadImportedModule: when the exception caught fromresolve()is aTerminationException, returnnullptrimmediately so the caller'sRETURN_IF_EXCEPTIONunwinds, instead of caching it as a resolution failure and proceeding intoFinishLoadingImportedModule. Both callers already check for a pending exception on the next line.Branch composition
This branch is currently based on
4895f45dfbd0(Bun's currentWEBKIT_VERSION) with #282, #306 and #286 cherry-picked, so the preview build oven-sh/bun#34655 points at pulls in only the termination fixes and not the unrelated JSMicrotask/JSPromise refactors between4895f45dfbd0andmain. #282 and #306 are already merged; #286 is Jarred's open PR that relaxes thecontinueDynamicImport/getOwnPropertyDescriptor/JSValue::getassertions for a pendingTerminationException. The hostLoadImportedModule change in the final commit is the only new code here.