Skip to content

ShadowRealm: cache importValue module in its own realm under require(esm) - #36321

Open
robobun wants to merge 2 commits into
mainfrom
farm/d889cf53/sync-queue-realm
Open

ShadowRealm: cache importValue module in its own realm under require(esm)#36321
robobun wants to merge 2 commits into
mainfrom
farm/d889cf53/sync-queue-realm

Conversation

@robobun

@robobun robobun commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Problem

require(esm) drives JSC's module loader through a VM-wide synchronous queue (vm.m_synchronousModuleQueue). If a ShadowRealm.importValue starts while that queue is active (e.g. from the top level of the required ESM), the ShadowRealm's module-loader reactions are diverted to the queue and then replayed against the caller's globalObject instead of the ShadowRealm's. The module is registered in the main realm's module map, so:

  • the second importValue for the same specifier loads a fresh instance instead of the cached one, and
  • a later import() in the outer realm sees the ShadowRealm's instance.
// trigger.mjs  (evaluated under loadModuleSync)
const realm = new ShadowRealm();
const first = realm.importValue(counterHref, "getCounter");
export { realm, first };

// entry.cjs
const { realm, first } = require("./trigger.mjs");
const a = await first;
const b = await realm.importValue(counterHref, "getCounter");
a(); // 0
b(); // 0, should be 1 (same module)
(await import(counterHref)).getCounter(); // 1, should be 0 (different realm)

#34121 wraps every CommonJS entry load in the same queue, which makes Node's test/parallel/test-shadow-realm-module.js fail there.

Cause

JSModuleLoader::drainSynchronousModuleQueue passes its caller's globalObject to runInternalMicrotask for every queued task. The four diversion points in JSPromise.cpp all have the reaction's own realm in scope (it is exactly what queueMicrotask would have used) but do not store it on SynchronousModuleTask.

Fix

JavaScriptCore side: oven-sh/WebKit#371 stores the realm on SynchronousModuleTask, replays against it in the drain loop (including the exception-path queueMicrotask), and marks it in VM::visitAggregateImpl.

This PR bumps WEBKIT_VERSION to that PR's preview build (parent is the currently pinned 549170099, so nothing else rides along) and adds a regression test.

Verification

$ USE_SYSTEM_BUN=1 bun test test/js/bun/jsc/shadow.test.js
(fail) importValue caches modules in the ShadowRealm when kicked off under require(esm)
  {
    "a0": 0,
-   "b0": 1,
-   "b1": 2,
-   "outer": 0,
+   "b0": 0,
+   "b1": 1,
+   "outer": 1,
  }

With the fix (local WebKit build):

(pass) shadow realm works
(pass) importValue caches modules in the ShadowRealm when kicked off under require(esm)

Also ran test/parallel/test-shadow-realm-module.js and the require-esm-* sync-queue tests; all pass.

Unblocks #34121 for test-shadow-realm-module.js.


[decide:webkit] gate passed · iteration 1 · 2 files touched

passes on PR (with fix)
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/js/bun/jsc/shadow.test.js'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/jsc/shadow.test.js
bun test v1.4.0 (64028c747)

test/js/bun/jsc/shadow.test.js:
(pass) shadow realm works [46.91ms]
(pass) importValue caches modules in the ShadowRealm when kicked off under require(esm) [593.39ms]

 2 pass
 0 fail
 5 expect() calls
Ran 2 tests across 1 file. [2.75s]
Exit: 0
diff hotspot
scripts/build/deps/webkit.ts   |  2 +-
 test/js/bun/jsc/shadow.test.js | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

gate history · 1 passed · 1 rejected · iteration 1

evidence per changed file
file                            reads  edits  tests
scripts/build/deps/webkit.ts        3      2      0
test/js/bun/jsc/shadow.test.js      1      1      0

…esm)

When require(esm) drives the module loader through the VM-wide synchronous
queue and a ShadowRealm.importValue starts during that drain, the ShadowRealm's
module-loader reactions were replayed against the caller's global. The first
import was registered in the main realm's module map instead of the
ShadowRealm's, so a second importValue for the same specifier fetched a fresh
instance and the module leaked into the outer realm's import().

The fix is in JavaScriptCore (oven-sh/WebKit#371): store the diverting realm on
SynchronousModuleTask and replay against it. This bumps WEBKIT_VERSION to that
PR's preview build and adds a regression test.
@robobun

robobun commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

The WebKit preview build for oven-sh/WebKit#371 (autobuild-preview-pr-371-4e9f0037) is published. Verified locally with bun bd test test/js/bun/jsc/shadow.test.js: fails without the fix, passes with it.

CI build 84921 finished: 192/196 passed. shadow.test.js is green on every lane that ran. Remaining red is infrastructure, not this diff:

  • test/js/bun/s3/s3.test.ts R2 upload timeout on one aarch64 shard
  • :darwin: 14 x64 buildkite agent couldn't clean its checkout dir (unlinkat ... directory not empty, git clone exit 128, never ran tests)
  • 2 × :darwin: 14 aarch64 shards expired waiting for an agent

Ready for review. WEBKIT_VERSION needs swapping to the merged main SHA once oven-sh/WebKit#371 lands.

@robobun

robobun commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 5:05 AM PT - Jul 29th, 2026

@robobun, your commit 64028c7 has 2 failures in Build #84921 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 36321

That installs a local version of the PR into your bun-36321 executable, so you can run:

bun-36321 --bun

@robobun
robobun marked this pull request as ready for review July 29, 2026 05:58
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: cbf599e0-7a77-4d33-9674-c61f1fe57d21

📥 Commits

Reviewing files that changed from the base of the PR and between 59242d6 and 64028c7.

📒 Files selected for processing (2)
  • scripts/build/deps/webkit.ts
  • test/js/bun/jsc/shadow.test.js

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

* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "549170099226f816a4b204ea1d8fa102fb79eefa";
export const WEBKIT_VERSION = "autobuild-preview-pr-371-4e9f0037";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-371-4e9f0037, an ephemeral preview tag that GitHub deletes when oven-sh/WebKit#371 merges or closes — at which point every fresh checkout/CI build 404s on the WebKit prebuilt download. Before merge, swap this to the merged main-branch SHA of oven-sh/WebKit (once #371 lands) and verify prebuilt artifacts exist for every platform × flavor.

Extended reasoning...

What the bug is

scripts/build/deps/webkit.ts:6 sets WEBKIT_VERSION = "autobuild-preview-pr-371-4e9f0037". This is an autobuild-preview-pr-* tag — a preview release published for an unmerged WebKit PR (oven-sh/WebKit#371), not a stable main-branch commit. The PR description itself acknowledges this: "This PR bumps WEBKIT_VERSION to that PR's preview build."

Why this is merge-blocking

The repo's own review rules in .claude/docs/landing-prs.md § Dependencies & vendoring state explicitly:

Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds) — swap to the merged upstream SHA and verify prebuilt artifacts exist for every platform × flavor before merge.

And the build system's own error handling confirms exactly what happens when this class of pin goes stale — scripts/build/download.ts:278-294:

The autobuild-preview-pr-* WebKit tags are the sharp edge: GitHub deletes the preview release when the PR merges or closes, so every build 404s at once. … Those releases only exist while the WebKit PR is open — this one has merged, closed, or been re-tagged.

Concrete failure walkthrough

  1. This PR merges to main with WEBKIT_VERSION = "autobuild-preview-pr-371-4e9f0037".
  2. SynchronousModuleQueue: replay diverted reactions against their own realm WebKit#371 merges (or closes, or gets a new preview build). GitHub deletes the autobuild-preview-pr-371-4e9f0037 release.
  3. A developer (or CI runner) does a fresh checkout with an empty cache and runs bun bd.
  4. prebuiltUrl() in webkit.ts computes https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-371-4e9f0037/bun-webkit-<os>-<arch><suffix>.tar.gz.
  5. The download returns HTTP 404. prebuiltDownloadError() throws "WebKit preview release is gone" and the build fails.
  6. Every fresh CI checkout on main is now broken until someone lands a follow-up commit changing WEBKIT_VERSION.

Nothing in the existing code prevents this — the dedicated error handler in download.ts exists precisely because this failure mode has bitten before; it makes the crash legible but does not avert it.

Impact

  • main becomes unbuildable from a clean cache the moment the upstream WebKit PR's lifecycle changes — a window that is entirely outside this repo's control.
  • Anyone bisecting through this commit range in the future will hit a build that can never be reproduced (the artifact is gone forever).

Fix

Before merging this PR:

  1. Land SynchronousModuleQueue: replay diverted reactions against their own realm WebKit#371 on oven-sh/WebKit main.
  2. Set WEBKIT_VERSION to the resulting 40-hex main-branch commit SHA (the previous value 549170099226f816a4b204ea1d8fa102fb79eefa is stated to be the parent, so only Fix typo #371's change rides along).
  3. Verify prebuilt tarballs exist under https://github.com/oven-sh/WebKit/releases/tag/autobuild-<sha> for every platform × {debug, lto, asan, musl} flavor the build matrix consumes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Correct, this is the staging state so CI can exercise the fix. Before merge this gets swapped to the 40-hex main SHA once oven-sh/WebKit#371 lands and its autobuild-<sha> release is published (the preview build parent is the currently pinned 549170099, so only that one commit rides along).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant