Skip to content

JSModuleLoader: don't treat a TerminationException from resolve() as a resolution failure #309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 4 commits into from
Closed

JSModuleLoader: don't treat a TerminationException from resolve() as a resolution failure #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a63be92
Defer termination at the reifyStaticProperty call sites and propagate…
robobun Jul 17, 2026
0006cf6
Drop the ThrowScope from the reifyStaticProperty call sites (#306)
robobun Jul 17, 2026
38ddd57
JSC: tolerate pending TerminationException in property-lookup assertions
Jarred-Sumner Jul 13, 2026
5113fb4
JSModuleLoader: don't treat a TerminationException from resolve() as …
robobun Jul 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions Source/JavaScriptCore/runtime/JSCJSValuePropertyInlines.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,14 @@ ALWAYS_INLINE JSValue JSValue::get(JSGlobalObject* globalObject, PropertyName pr

ALWAYS_INLINE JSValue JSValue::get(JSGlobalObject* globalObject, PropertyName propertyName, PropertySlot& slot) const
{
auto scope = DECLARE_THROW_SCOPE(getVM(globalObject));
VM& vm = getVM(globalObject);
auto scope = DECLARE_THROW_SCOPE(vm);
bool hasSlot = getPropertySlot(globalObject, propertyName, slot);
EXCEPTION_ASSERT(!scope.exception() || !hasSlot);
// A DeferTermination scope unwinding inside the slot lookup can throw the
// TerminationException after the property was found; tolerate it like
// JSObject::get does and let the caller's exception check unwind.
EXCEPTION_ASSERT(!scope.exception() || vm.hasPendingTerminationException() || !hasSlot);
RETURN_IF_EXCEPTION(scope, jsUndefined());
if (!hasSlot)
return jsUndefined();
RELEASE_AND_RETURN(scope, slot.getValue(globalObject, propertyName));
Expand Down Expand Up @@ -136,7 +141,10 @@ ALWAYS_INLINE JSValue JSValue::get(JSGlobalObject* globalObject, unsigned proper
object = asObject(asCell());

bool hasSlot = object->getPropertySlot(globalObject, propertyName, slot);
EXCEPTION_ASSERT(!scope.exception() || !hasSlot);
// See JSValue::get(JSGlobalObject*, PropertyName, PropertySlot&) above:
// termination can be thrown after a successful lookup.
EXCEPTION_ASSERT(!scope.exception() || getVM(globalObject).hasPendingTerminationException() || !hasSlot);
RETURN_IF_EXCEPTION(scope, jsUndefined());
if (!hasSlot)
return jsUndefined();
RELEASE_AND_RETURN(scope, slot.getValue(globalObject, propertyName));
Expand Down
10 changes: 9 additions & 1 deletion Source/JavaScriptCore/runtime/JSModuleLoader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,11 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject,
resolved = resolve(globalObject, specifier, referrerKey, scriptFetcher, useImportMap);
// 9. If the previous step threw an exception, then:
if (Exception* resolutionError = scope.exception()) {
// A TerminationException is not a resolution failure; bail without
// caching it or entering FinishLoadingImportedModule with it still
// pending on the VM.
if (vm.isTerminationException(resolutionError)) [[unlikely]]
return nullptr;
attachErrorInfo(globalObject, resolutionError, nullptr, specifier, moduleRequest.type(), ModuleFailure::Kind::Instantiation);
// Cache the resolution error so subsequent calls for the same specifier return the same error object.
JSValue errorValue = resolutionError->value();
Expand Down Expand Up @@ -1023,7 +1028,10 @@ void JSModuleLoader::continueDynamicImport(JSGlobalObject* globalObject, ModuleL
// 1.a. Perform ! Call(promiseCapability.[[Reject]], undefined, « moduleCompletion.[[Value]] »).
promise->reject(vm, (*exception)->value());
// 1.b. Return UNUSED.
scope.assertNoException();
// 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();
Comment on lines +1031 to +1034

@coderabbitai coderabbitai Bot Jul 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
// 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.

Comment on lines +1031 to +1034

@claude claude Bot Jul 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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):

  1. Worker termination fires while hostLoadImportedModule is inside resolve().
  2. resolve() returns with TerminationException pending; scope.exception() yields it.
  3. New guard: vm.isTerminationException(resolutionError) → return nullptr.
  4. Caller's RETURN_IF_EXCEPTION unwinds. finishLoadingImportedModule / continueDynamicImport are 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.

return;
}
// 2. Let module be moduleCompletion.[[Value]].
Expand Down
16 changes: 14 additions & 2 deletions Source/JavaScriptCore/runtime/JSObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2948,6 +2948,10 @@ void JSObject::reifyAllStaticProperties(JSGlobalObject* globalObject)
if (!structure()->isDictionary())
convertToDictionary(vm);

// A PropertyCallback builder can enter JS; defer termination (like
// LazyProperty::callFunc) so it can't return with one pending. No
// ThrowScope here: JSObject::deleteProperty reaches this without one.
DeferTerminationForAWhile deferScope(vm);
for (const ClassInfo* info = classInfo(); info; info = info->parentClass) {
const HashTable* hashTable = info->staticPropHashTable;
if (!hashTable)
Expand All @@ -2957,8 +2961,12 @@ void JSObject::reifyAllStaticProperties(JSGlobalObject* globalObject)
unsigned attributes;
auto key = Identifier::fromString(vm, value.m_key);
PropertyOffset offset = getDirectOffset(vm, key, attributes);
if (!isValidOffset(offset))
if (!isValidOffset(offset)) {
reifyStaticProperty(vm, hashTable->classForThis, key, value, *this);
// Leave the rest lazy on throw; the caller propagates.
if (vm.exceptionForInspection()) [[unlikely]]
return;
}
}
}

Expand Down Expand Up @@ -3933,7 +3941,11 @@ bool JSObject::getOwnPropertyDescriptor(JSGlobalObject* globalObject, PropertyNa
PropertySlot slot(this, PropertySlot::InternalMethodType::GetOwnProperty);

bool result = methodTable()->getOwnPropertySlot(this, globalObject, propertyName, slot);
EXCEPTION_ASSERT_UNUSED(scope, !scope.exception() || !result);
// A DeferTermination scope unwinding inside the slot lookup can throw the
// TerminationException after the property was found; tolerate it like
// JSObject::get does and let the caller's exception check unwind.
EXCEPTION_ASSERT(!scope.exception() || vm.hasPendingTerminationException() || !result);
RETURN_IF_EXCEPTION(scope, false);
if (!result)
return false;

Expand Down
15 changes: 14 additions & 1 deletion Source/JavaScriptCore/runtime/Lookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "config.h"
#include "Lookup.h"

#include "DeferTermination.h"
#include "GetterSetter.h"
#include "JSCInlines.h"
#include <wtf/text/MakeString.h>
Expand Down Expand Up @@ -58,7 +59,19 @@ bool setUpStaticFunctionSlot(VM& vm, const ClassInfo* classInfo, const HashTable
if (thisObject->staticPropertiesReified())
return false;

reifyStaticProperty(vm, classInfo, propertyName, *entry, *thisObject);
{
// A PropertyCallback builder can enter JS; defer termination (like
// LazyProperty::callFunc) so it can't return with one pending.
DeferTerminationForAWhile deferScope(vm);
reifyStaticProperty(vm, classInfo, propertyName, *entry, *thisObject);
}
// The builder may still throw a non-termination exception; report the
// slot as not found so JSValue::get / getOwnPropertyDescriptor's
// EXCEPTION_ASSERT(!scope.exception() || !result) holds. No ThrowScope
// here: a ThrowScope would simulate a throw on every first static-table
// lookup, and callers of getOwnPropertySlot don't check for one.
if (vm.exceptionForInspection()) [[unlikely]]
return false;

offset = thisObject->getDirectOffset(vm, propertyName, attributes);
if (!isValidOffset(offset)) {
Expand Down
5 changes: 5 additions & 0 deletions Source/JavaScriptCore/runtime/Lookup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,11 @@ inline void reifyStaticProperty(VM& vm, const ClassInfo* classInfo, const Proper

if (value.attributes() & PropertyAttribute::PropertyCallback) {
JSValue result = value.lazyPropertyCallback()(vm, &thisObj);
// 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;
Comment on lines +541 to +545

@claude claude Bot Jul 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. setUpStaticFunctionSlot (Lookup.cpp:66) — now wraps in DeferTerminationForAWhile and checks vm.exceptionForInspection() afterwards.
  2. JSObject::reifyAllStaticProperties (JSObject.cpp:2965) — same treatment.
  3. reifyStaticProperties<ArrayType> (Lookup.h:~569-580) — the template at the bottom of this same header, which loops over a HashTableValue array and calls reifyStaticProperty for each entry with no DeferTerminationForAWhile scope 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:

  1. reifyStaticProperty is called for entry N; the callback returns an empty JSValue with an exception on the VM.
  2. The new if (!result) return; guard fires (a strict improvement — before this PR it would have crashed in putDirect on an empty value).
  3. Control returns to the for loop in reifyStaticProperties<ArrayType>, which does not inspect vm.exceptionForInspection().
  4. The loop proceeds to entry N+1 and calls reifyStaticProperty again 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.

thisObj.putDirect(vm, propertyName, result, attributesForStructure(value.attributes()));
return;
}
Expand Down
Loading