[JSC] DFG folds a module variable to a constant when an import cycle links a function that writes it before the module's own code

[robobun]

Problem

• A module-scope variable silently freezes once its users reach DFG: reads fold to a stale constant and writes vanish. The trigger is an import cycle in which another module calls one of the module's hoisted functions before the module's own code runs. Default options, about 500 calls. Node is correct.
• DFG folds a closure variable while its SymbolTableEntry watchpoint set is IsWatched (Graph::tryGetConstantClosureVar). A ClosureVar op_put_to_scope never notifies that set. Linking the put invalidates it instead (CodeBlock::finishCreation: if (op.watchpointSet) op.watchpointSet->invalidate(...)).
• The set is created lazily, when the code that declares the variable links. In a cycle the function links first, the entry has no set, and nothing is invalidated. The module code then creates the set, its first store makes it IsWatched, and it stays that way.

Fix

• abstractAccess() (runtime/JSScope.cpp) calls entry.prepareToWatch() for a ClosureVar Put, so the set exists when CodeBlock::finishCreation invalidates it.
• Correct because it restores the rule DFG relies on: every put notifies the set or invalidated it at link time. Only modules can link in this order. Function and program scopes link their declaring code first.
• Verified: JSTests/modules/import-cycle-function-linked-before-module-code.js (new, let and var). An unpatched shell fails all 11 module configurations, a patched shell passes all 11.

Background

• A variable's watchpoint set goes Clear, IsWatched on the first write, IsInvalidated on the second. DFG treats an IsWatched variable as a constant.
• A CodeBlock links on its first call. Linking resolves each scope access against the real scope chain and caches the result in the instruction's metadata.
• ES module function declarations are created when the module links, before any module code runs. In a cycle they are callable while the module's let bindings are still in their TDZ.

Notes

Reported with the patch by @dylan-conway. Reproduces on Bun 1.3.13 and on the plain jsc shell built from this tree. The Bun side is oven-sh/bun#42321: it pins this PR's preview build and adds a regression test to Bun's own suite. abstractAccess() and the op_put_to_scope link code are byte-identical in upstream WebKit main, so Safari should have the same bug and the patch applies there unchanged.

Repro (jsc -m main.mjs prints BAD ... unpatched and OK patched, --useDFGJIT=false is always OK):

// a.mjs
import "./b.mjs";
let hits = 0;
export function bump(n) { if (n) hits += n; return hits; }
export function three() { let r = []; for (let i = 0; i < 3; i++) r.push(bump(1)); return r.join(","); }
export function read() { return hits; }

// b.mjs: evaluates before a.mjs, and calls (therefore links) bump() first
import * as a from "./a.mjs";
try { a.bump(0); } catch {}

// main.mjs
const a = await import("./a.mjs");
let expect = 0, bad = 0, first;
for (let i = 0; i < 100000; i++) {
  const got = a.three(), want = [expect + 1, expect + 2, expect + 3].join(",");
  if (got !== want || a.read() !== expect + 3) { bad++; first ??= `round ${i}: ${got} want ${want} read=${a.read()}`; }
  expect = a.read();
}
print(bad ? "BAD " + bad + " first: " + first : "OK");

• Why the other tiers do not save it: LLInt and Baseline putClosureVar store without a notify, and the DFG parser reads the watchpoint set from the put's metadata only for ResolvedClosureVar, GlobalVar and GlobalLexicalVar. A ClosureVar put relies on the link-time invalidation alone.
• Readers that fold: a closure read in the same module, an imported binding (a ModuleVar get links as a ClosureVar get on the exporter's environment), and a namespace object property (DFGByteCodeParser.cpp, the getById.moduleEnvironment() path). The test checks all three.
• Other writers are fine. symbolTablePut looks the set up on every call. Global var and global lexical entries call prepareToWatch() when they are declared. The bytecode optimizer leaves put_to_scope dynamic for this reason (BytecodeOptimizer.cpp: "linking it also invalidates the variable's watchpoint").
• A Get is unchanged. abstractResolve(..., Put, ...) has one caller, the op_put_to_scope case in CodeBlock::finishCreation.
• Cost: a function-scope var that its declaring function never assigns, and an inner function does, now gets a 16-byte FatEntry that is invalidated at once. It was not foldable before either.
• prepareToWatch() runs under symbolTable->m_lock, as the ResolvedClosureVar case in CodeBlock::finishCreation does.
• Test runs: a local Release + asserts jsc from this tree, with and without the patch, through Tools/Scripts/run-jsc-stress-tests.
  • JSTests/modules.yaml (111 files, 11 configurations): the only difference is the new test, 11 failures to 11 passes. 8 files fail on both shells (they need $vm, or they expect upstream behaviour).
  • JSTests/stress, the 287 files that match closure|scope|watchpoint|symbol-table|tdz|captured|put-to|lexical|let-|const-|var-inject|activation (4433 runs): identical results, 1 file fails on both.
• The test sizes its loop with testLoopCount. With --report-execution-time the slowest of the 11 configurations takes 122 ms on the patched Release shell. On the unpatched shell the default configuration fails 30 of 30 runs.
• JSTests/ is in the sparse-checkout exclude list of .github/workflows/build-reusable.yml, so this repo's CI builds the shell and does not run the new test.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 1717053f-85e8-44bf-b9ed-5df68a3ce32d

📥 Commits

Reviewing files that changed from the base of the PR and between 4b9ff99 and 628f692.

📒 Files selected for processing (4)

• JSTests/modules/import-cycle-function-linked-before-module-code.js
• JSTests/modules/import-cycle-function-linked-before-module-code/1.js
• JSTests/modules/import-cycle-function-linked-before-module-code/2.js
• Source/JavaScriptCore/runtime/JSScope.cpp

Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review.

---

Walkthrough

The change updates lexical variable watchpoint handling in JavaScriptCore and adds cyclic module tests. The tests cover linked function calls before module evaluation, TDZ behavior, hoisted bindings, direct and namespace imports, and repeated counter updates.

Changes

Module cycle binding synchronization

Layer / File(s)	Summary
Cyclic module bindings and helper functions JSTests/modules/import-cycle-function-linked-before-module-code/*.js	The modules create a cyclic import. Module 1 exports lexical and hoisted counters with update and read functions. Module 2 checks calls made before Module 1 evaluation.
Lexical Put watchpoint preparation Source/JavaScriptCore/runtime/JSScope.cpp	Lexical Put operations prepare the symbol-table entry watchpoint before generating the closure-variable resolve operation.
Repeated cycle binding validation JSTests/modules/import-cycle-function-linked-before-module-code.js	The stress test validates imported functions, namespace bindings, read helpers, single updates, and repeated updates across 100,000 iterations.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 628f6

The module-cycle watchpoint fix and its targeted regression coverage do not leave a concrete merge-blocking risk.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the JSC DFG bug, the affected module variable, and the import-cycle trigger. It is specific and related to the main change.
Description check	✅ Passed	The description provides a detailed problem statement, fix explanation, technical background, regression tests, and validation results. It does not include the required WebKit Bugzilla link, Reviewed …

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.

---

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

[claude]

Thanks for switching the driver to testLoopCount — that addresses the only note from the earlier pass. I re-reviewed and didn't find any bugs; because this is a JIT-correctness change to the ClosureVar watchpoint lifecycle in runtime/JSScope.cpp (paths owned by jsc-reviewers in CODEOWNERS), a human JSC reviewer should still sign off.

What was reviewed

• abstractAccess() change: prepareToWatch() runs under the existing symbolTable->m_lock locker and only on the Put path, feeding entry.watchpointSet() into the ResolveOp so CodeBlock::finishCreation can invalidate it — matches the ResolvedClosureVar pattern it mirrors.
• SymbolTableEntry::prepareToWatch() is a no-op when the entry is already fat or unwatchable, so repeated/late calls are safe.
• New module regression test: import cycle triggers the pre-link call, asserts via shouldBe/shouldThrow, iterates testLoopCount times, and covers direct binding, namespace, and reader-function paths for both let and var.

Extended reasoning...

Overview

The PR adds a two-line fix in Source/JavaScriptCore/runtime/JSScope.cpp: when abstractAccess() resolves a Put to a ClosureVar in a JSLexicalEnvironment, it now calls entry.prepareToWatch() before constructing the ResolveOp. This ensures the SymbolTableEntry's watchpoint set exists at link time so CodeBlock::finishCreation can invalidate it, closing a hole where an import cycle links a module's function (and its op_put_to_scope) before the module code that would otherwise create the set. Three new files under JSTests/modules/ add a regression test exercising the cycle across all reader shapes (closure read, imported binding, namespace property) for both let and var exports.

Security risks

None identified. The change does not touch auth, crypto, permissions, or untrusted input parsing. prepareToWatch() only inflates a symbol-table entry to allocate a WatchpointSet; it is invoked under the existing ConcurrentJSLocker on symbolTable->m_lock, matching how other callers use it. The worst-case downside noted by the author is a small per-entry allocation for function-scope vars that only inner functions assign — a memory cost, not a safety issue.

Level of scrutiny

High. Although the code delta is tiny, it sits on the boundary between scope resolution, CodeBlock linking, and DFG constant-folding invariants — an area where subtle ordering bugs manifest as silent miscompilations rather than crashes. Both changed paths (Source/JavaScriptCore/** and JSTests/**) are covered by .github/CODEOWNERS (@ WebKit/jsc-reviewers), which per the approval guidelines means this should not be auto-approved.

Other factors

Since the prior review, the author pushed a commit replacing the hardcoded 1e5 loop with testLoopCount, addressing the only inline note. The bug-hunting pass exited on a dry streak with no findings. The PR description includes stress-test parity results and an unpatched-vs-patched failure/pass demonstration for the new test, and the fix mirrors the established prepareToWatch()-before-watchpointSet() pattern used elsewhere (e.g., the ResolvedClosureVar handling in CodeBlock::finishCreation). Given all feedback is addressed and no new issues surfaced, the remaining reason to defer is CODEOWNERS coverage and the inherent subtlety of watchpoint-lifecycle changes.

[github-actions]

Preview Builds

Commit	Release	Date
d167f156	autobuild-preview-pr-624-d167f156	2026-09-11 11:39:43 UTC