[JSC] AbstractModuleRecord::link: one throw scope for the whole function

[dylan-conway]

What

AbstractModuleRecord::link has a Bun-only block (added in #662) that opens a throw scope, runs generateDeferredSyntheticModules(), checks, and closes the scope. It then calls CyclicModuleRecord::link(), which opens a throw scope of its own.

A throw scope that closes leaves its caller owing an exception check. Nothing checks before the next scope is constructed, so with exception-check validation on (validateExceptionChecks, which Bun's ASAN lane enables) every module link aborts:

ERROR: Unchecked JS exception:
    This scope can throw a JS exception: link @ Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp:1498
    But the exception was unchecked as of this scope: link @ Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp:541
ASSERTION FAILED: exception check validation failed

A two-file program is enough (import { x } from "./dep.mjs").

Change

Declare the scope once for the function, and check it after each link() call. No behaviour change otherwise.

Tested

JavaScriptCore built from this branch with Bun built against it, BUN_JSC_validateExceptionChecks=1: the two-file import links and runs; an import of a missing export reports its SyntaxError; Bun's module-loading tests pass.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 95e2ce6b-400d-414f-ab29-ff27e12931bf

📥 Commits

Reviewing files that changed from the base of the PR and between 2eb832d and c901df1.

📒 Files selected for processing (1)

• Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

---

Walkthrough

AbstractModuleRecord::link now reuses one VM throw scope across deferred synthetic generation and module linking. Cyclic and synthetic paths explicitly check and propagate exceptions before returning.

Changes

Module linking

Layer / File(s)	Summary
Link throw-scope and return flow Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp	link creates a shared VM and throw scope before deferred synthetic generation. Cyclic and synthetic linking paths now perform explicit exception checks and return after propagating exceptions.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

I reviewed the final version of this change (after the switch from scope.release() to a check after each link()) and found no bugs; since it changes the exception-scope contract of a function every module link goes through, in the CODEOWNERS-covered JavaScriptCore runtime, a human look is still worthwhile.

What was reviewed:

• Traced the ThrowScope lifecycle under ENABLE(EXCEPTION_SCOPE_VERIFICATION): the old inner Bun-only scope's destructor calls simulateThrow(), so the next DECLARE_THROW_SCOPE in CyclicModuleRecord::link tripped verifyExceptionCheckNeedIsSatisfied; one function-wide scope with RETURN_IF_EXCEPTION after each dispatch removes that.
• Checked every caller of AbstractModuleRecord::link (JSModuleLoader.cpp:464, JSMicrotask.cpp:1389 and :1492, CyclicModuleRecord.cpp:428, JSWebAssemblyInstance.cpp:246): each checks scope.exception() or RETURN_IF_EXCEPTION right after, so the outer scope's own simulated throw is satisfied.
• Non-Bun builds now also get a scope in this function; that is harmless, since SyntheticModuleRecord::link cannot throw and the extra check on that path is a no-op.

Extended reasoning...

Overview

The PR changes one function, AbstractModuleRecord::link in /home/claude/webkit/Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp. The VM& and DECLARE_THROW_SCOPE that previously lived only inside the #if USE(BUN_JSC_ADDITIONS) block are hoisted to function scope, and each of the two dispatch calls (CyclicModuleRecord::link, SyntheticModuleRecord::link) is followed by RETURN_IF_EXCEPTION(scope, void()). The three commits show the author trying scope.release() first and then replacing it with explicit checks; the final diff contains no release().

Security risks

None specific to this change. It does not alter control flow in release builds beyond an early return on an already-pending exception (which the callee's own scope already implied), and it does not touch input parsing, permissions, or memory ownership. In debug/ASAN builds it removes a spurious assertion abort during module linking.

Level of scrutiny

Moderate, and I applied it. The correctness question is entirely about ThrowScope bookkeeping. Reading /home/claude/webkit/Source/JavaScriptCore/runtime/ThrowScope.cpp: ThrowScope::~ThrowScope calls simulateThrow() unless returning into LLInt/JIT, which sets m_needExceptionCheck; the next ThrowScope constructor calls verifyExceptionCheckNeedIsSatisfied. So the old inner block's scope destructing right before CyclicModuleRecord::link opened its scope is exactly the failure the PR description reports. With one outer scope, the callee's scope destructor marks a need, RETURN_IF_EXCEPTION satisfies it, then the outer scope's destructor marks a need for the caller. I confirmed all five callers of AbstractModuleRecord::link check immediately afterward (scope.exception() or RETURN_IF_EXCEPTION), so no new unchecked-exception site is introduced. SyntheticModuleRecord::link is a trivial return Synchronousness::Sync with no scope, so the check on that path is harmless and the discarded return value is unchanged behaviour. Non-Bun builds also gain the scope; that matches the pattern JSC uses elsewhere (e.g. JSWebAssemblyInstance.cpp:246-247) and adds no risk.

Other factors

The bug hunt exited on dry_streak with no findings and no ruled-out candidates. The change cannot be exercised by CI's tested lanes (release/LTO, no assertions), so the fix is verified only by reasoning and the author's reported local run with validateExceptionChecks. /home/claude/webkit/.github/CODEOWNERS (inherited from upstream) lists /Source/JavaScriptCore under a reviewer team, which under the approval guidelines rules out an automated approve; combined with the prior review on an earlier push of this PR, defer with a summary of what was verified is the appropriate level of signal.

[github-actions]

Preview build of c901df1: autobuild-preview-pr-688-c901df12