test: regression for require(esm) abort on react + @mui/material@9 Typography (#30281) - #30283
test: regression for require(esm) abort on react + @mui/material@9 Typography (#30281)#30283robobun wants to merge 1 commit into
Conversation
|
Updated 11:10 PM PT - May 5th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 30283That installs a local version of the PR into your bun-30283 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA new regression test for issue 30281 is added that reproduces a CommonJS-to-ESM module loading scenario by creating a temporary project, installing dependencies, executing require on an ESM module, and validating successful execution or gracefully skipping on known WebKit bugs. ChangesRegression Test for Issue 30281
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/build/deps/webkit.ts`:
- Line 6: The cache key derived from WEBKIT_VERSION in prebuiltDestDir() is too
short (slice(0, 16)) and causes collisions for autobuild-preview-* versions;
update prebuiltDestDir() to use a non-truncated identifier (e.g., include the
full WEBKIT_VERSION string or a stable hash of WEBKIT_VERSION) so each preview
build produces a unique cache directory; reference WEBKIT_VERSION and
prebuiltDestDir() when making the change.
In `@test/regression/issue/30281.test.ts`:
- Line 94: Remove the brittle assertion that stderr is empty in the regression
test: delete or disable the expect(stderr).toBe("") check in
test/regression/issue/30281.test.ts (the assertion shown in the diff) so the
test no longer fails on benign ASAN warnings; keep the other assertions (e.g.,
on stdout/exit code) intact or replace this line with a non-failing check if you
need to record stderr without making the test fail.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b44cfa38-772d-4827-ba7c-79c48feb449a
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/regression/issue/30281.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
02c689a to
82a73b7
Compare
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 `@test/regression/issue/30281.test.ts`:
- Line 66: The test uses loose package specifiers in the cmd array (cmd:
[bunExe(), "add", "react@19", "@mui/material@9", "@emotion/styled",
"@emotion/react"]) which can make the test flaky; update that cmd to pin exact
package versions for react and `@mui/material` and at minimum pin `@emotion/styled`
and `@emotion/react` to their known-good major versions (or exact patch versions)
so the install step is deterministic—locate the cmd array in the test and
replace the unpinned specifiers with exact version strings.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51ba9814-c607-4fcf-8093-34d5ba8dac87
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/regression/issue/30281.test.ts
858907b to
ee265c0
Compare
ee265c0 to
d8337c5
Compare
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 `@scripts/build/deps/webkit.ts`:
- Line 6: The exported WEBKIT_VERSION currently contains a tag-like ref (export
const WEBKIT_VERSION) which breaks the equality check in
scripts/sync-webkit-source.ts that compares git rev-parse HEAD (commit hash) to
WEBKIT_VERSION; change the workflow so the value compared is a commit SHA:
either resolve the tag/ref to its commit hash before assigning WEBKIT_VERSION
(e.g., call git rev-parse <ref> and store that hash) or update the sync logic in
scripts/sync-webkit-source.ts to run git rev-parse on WEBKIT_VERSION and compare
hashes; specifically ensure the symbol WEBKIT_VERSION holds a full commit SHA
(or compare git rev-parse HEAD to git rev-parse WEBKIT_VERSION) so the equality
check can succeed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d2a4bd3-95fa-4029-911c-993b864346e6
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/regression/issue/30281.test.ts
d8337c5 to
eda88de
Compare
robobun
changed the title
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 `@test/regression/issue/30281.test.ts`:
- Around line 100-117: The test currently treats the known crash signature as a
pass by returning early (variables isAbort and assertionPresent detect exitCode
134/133 and stderr.includes("ASSERTION FAILED: m_status == Status::Fetching")),
which hides regressions; change the early return to an explicit failing or skip
behavior: either throw a clear Error with a message referencing issue `#30281`
when isAbort && assertionPresent, or call the test framework's skip/skipTest
utility so the test is explicitly skipped rather than silently passing, ensuring
the detection logic (isAbort, assertionPresent) remains but no longer converts
the buggy crash into a green test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 958be4f2-ff99-4868-9fcb-339a8f5f9ee1
📒 Files selected for processing (1)
test/regression/issue/30281.test.ts
eda88de to
f259409
Compare
f259409 to
c64d715
Compare
There was a problem hiding this comment.
LGTM — test-only addition following the established split-PR pattern; the two inline nits are non-blocking polish.
Extended reasoning...
Overview
This PR adds a single new file, test/regression/issue/30281.test.ts, which pins a regression for a require(esm) abort in the WebKit module loader. It is the test half of a split-PR pair with oven-sh/WebKit#217 and follows the same auto-skip-until-WebKit-bump pattern established in #30186. No production code is touched — the scripts/build/deps/webkit.ts changes discussed earlier in the thread were dropped, and the final diff is test-only.
Security risks
None. The change is confined to the test suite: it creates a tempdir, runs bun add against pinned npm package majors, and spawns a subprocess via bunExe() with bunEnv. No auth, crypto, permissions, network listeners, or user-input handling is involved.
Level of scrutiny
Low-to-moderate. This is a new regression test using standard harness primitives (tempDir, bunExe, bunEnv, spawn/spawnSync, test.skipIf). The auto-skip-on-known-crash pattern is precedented and was explicitly validated in this thread. The PR has already been through five rounds of inline review (preview-tag handling, emotion peer pinning, typeof Typography brittleness, release-vs-debug stderr gating, Windows/musl exit-code shapes), and every substantive point was addressed with a follow-up commit and resolved.
Other factors
The two remaining bug-hunter findings posted as inline comments are both self-described nits with no runtime or test-result impact: (1) binding stderr so non-abort failures surface diagnostics in CI, and (2) the musl skip comment misattributes the Alpine failure to a non-existent native @emotion/is-prop-valid prebuilt rather than the CI runner's core-dump detection. Both are worth a follow-up but neither affects the test's correctness or its ability to guard the regression, so they don't block approval of a test-only PR.
…pography (#30281) Fixes #30281 — test side of the split-PR pair with oven-sh/WebKit#217. ## The bug `require()` of an ESM file that statically imports `react` + an MUI v9 forwardRef sub-module (e.g. `@mui/material/Typography`, `@mui/material/DialogContent`) aborts on the `m_status == Status::Fetching` assertion at `vendor/WebKit/Source/JavaScriptCore/runtime/ModuleRegistryEntry.cpp:254` — SIGABRT on Linux, arm64 PAC IB trap (SIGTRAP) on macOS. Running the same file as the ESM entry (`bun repro.js`) is fine; the bug is on the CommonJS-`require()`-of-ESM path added by #29393. Reporter's bisect: first bad `e2017e79956d0a040fcaab15071da5beac474f7f` (2026-04-26); last good `def567677b876d32176b5e16648408c9f0fefabc` (2026-04-24); range includes #29393. ## Root cause `moduleRegistryModuleSettled` (`JSMicrotask.cpp:866`) fires twice for the same `ModuleRegistryEntry`: once inline through `hostLoadImportedModule`'s synchronous-replay branch (`JSModuleLoader.cpp:712-725`, the _fetchPromise is Fulfilled_ case), once via the stale `ModuleRegistryModuleSettled` reaction that `moduleRegistryFetchSettled` had queued on the normal microtask queue before the `require(esm)` entered sync mode. `moduleRegistryFetchSettled` already guards this exact shape: if (modulePromise->status() != JSPromise::Status::Pending) return; `moduleRegistryModuleSettled` did not. ## Fix WebKit-side — oven-sh/WebKit#217 — applies the same pending-guard symmetrically to `moduleRegistryModuleSettled`. Bun-side activates automatically once `WEBKIT_VERSION` is next bumped past that merge; until then the test probes the crash shape (exit 134/133 + `ASSERTION FAILED: m_status == Status::Fetching` on stderr) and short-circuits CI-green, matching the split pattern used by #30186 for oven-sh/WebKit#214. ## Tests `test/regression/issue/30281.test.ts` installs `react@19` + `@mui/material@9` + `@emotion/styled@11` + `@emotion/react@11` (emotion pinned to what MUI v9 peerDepends on so a future emotion major can't reshape the graph) in a tempdir, runs `bun -e 'require("./repro.js")'`, and either (a) asserts the fixed behaviour when the WebKit fix has landed or (b) logs+skips when the bug is still present. Any other failure mode falls through to the assertions so install/peer-dep regressions don't get silently swallowed.
c64d715 to
789c609
Compare
There was a problem hiding this comment.
LGTM — test-only regression addition; all prior review feedback addressed (the isMusl skip + incorrect rationale was replaced with the ulimit -c 0 wrapper, which is the right fix for the core-dump-detection issue I flagged).
Extended reasoning...
Overview
Single new file: test/regression/issue/30281.test.ts (153 lines). Adds a regression test for the require(esm) → m_status == Status::Fetching abort with react + @mui/material@9. The actual C++ fix is in oven-sh/WebKit#217; this is the test side of the split-PR pattern (precedent: #30186 / oven-sh/WebKit#214). The test installs real npm packages into a tempdir, runs bun -e 'require("./repro.js")' via a /bin/sh -c 'ulimit -c 0 && exec …' wrapper, and auto-skips on the known abort exit codes (134/133) until WEBKIT_VERSION is bumped past the WebKit fix.
Security risks
None. Test-only; spawns bunExe() in a tempdir with pinned major-version npm packages. No auth, crypto, permissions, or production code paths touched.
Level of scrutiny
Low-to-moderate. Test-only addition with no runtime/build changes (the earlier scripts/build/deps/webkit.ts edits were dropped from the final diff). Worst-case failure mode is a flaky/red CI lane, not a shipped bug. The test follows established repo conventions: tempDir + using, bunExe()/bunEnv, exit-code-last assertion order, the #29322 conditional-stderr house style, and the #30205 ulimit -c 0 precedent for suppressing expected core dumps.
Other factors
This PR went through extensive iteration. Every concern I raised across prior rounds was addressed: emotion peers pinned to @11, Typography != null instead of typeof === "object", auto-skip loosened for release builds (no stderr assertion text), test.skipIf(isWindows) for NTSTATUS exit codes, stderr captured and surfaced on non-abort failure. My last unresolved nit (comment 3192311707, about the factually-wrong @emotion/is-prop-valid musl-prebuilt rationale) was resolved by removing the isMusl skip entirely and instead wrapping the subprocess in ulimit -c 0 — which directly addresses the real cause I identified (scripts/runner.node.mjs flagging the core file) and restores Alpine coverage rather than skipping it. That's a strictly better outcome than my suggested comment-text fix. No bugs found by the bug-hunting system on the final revision. The commit (789c609) is already on main.
|
Another robobun run landed on the same bug from a different reporter (issue #30493 — diamond ESM via barrel where shared.js imports |
- Replace manual setTimeout/clearTimeout with Bun.spawn's native timeout option (matches spawn-pipe-read-error-leak.test.ts pattern). - Drop the negative 'ASSERTION FAILED' stderr check; stdout snapshot + signalCode + exitCode cover both the abort and deadlock failure modes. - Fix stale comment: 30281.test.ts was never landed (this test subsumes the react+MUI repro from #30283), and the fix is WebKit#225 not #217.
…0527) ## WebKit changes (88b2f7a2 → 5488984d) Single commit on top of the previous pin: ### `module-loader: don't double-fire moduleRegistryModuleSettled after inline sync replay` (oven-sh/WebKit#225, rebased #217) **File:** `Source/JavaScriptCore/runtime/JSMicrotask.cpp` **What:** Adds a `modulePromise->status() != Pending` early-return guard to `moduleRegistryModuleSettled`, symmetric with the guard already present in `moduleRegistryFetchSettled`. Gated under `#if USE(BUN_JSC_ADDITIONS)`. **Why:** `require()` of an ESM whose graph contained a diamond dependency through a barrel deadlocked (release) / aborted on `ASSERTION FAILED: m_status == Status::Fetching` (debug). `hostLoadImportedModule`'s synchronous-replay branch (taken when `require(esm)` is draining the synchronous module queue) calls `fetchComplete` + fulfills `modulePromise` inline. If a `ModuleRegistryFetchSettled` reaction had already run on the *normal* microtask queue for the same entry before sync mode was entered, it left a stale `ModuleRegistryModuleSettled` reaction queued there. When the normal queue later drained, that reaction re-entered `fetchComplete` on an already-`Fetched` entry. No changes to `JSType.h`. No WebCore code-generator changes. --- **Verification:** - ✅ `test/regression/issue/30493.test.ts` fails on current `main` (assertion crash, empty stdout) - ✅ Same test passes on `bun run build:local` with the patched WebKit - ✅ Same test passes on `bun bd` with the prebuilt preview tarball - ✅ Full bun CI green against `autobuild-preview-pr-225-2b6b1c39` (build #53556 — 67 pass, 3 pre-existing main flakes also red on #30522) Fixes #30493 Fixes #30281 Closes #30283 (the dependency-free 6-file repro in this PR covers the same root cause without needing a react+MUI install) --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
1 participant
Fixes #30281. Test side of the split-PR pair with oven-sh/WebKit#217 (the actual fix).
What
require()of an ESM file that statically importsreact+ an MUI v9 forwardRef sub-module (e.g.@mui/material/Typography,@mui/material/DialogContent) aborts on them_status == Status::Fetchingassertion atvendor/WebKit/Source/JavaScriptCore/runtime/ModuleRegistryEntry.cpp:254— SIGABRT on Linux, arm64 PAC IB trap (SIGTRAP) on macOS. Running the same file as the ESM entry (bun repro.js) is fine; the bug is on the CommonJS-require()-of-ESM path added by #29393.Reporter's bisect: first bad
e2017e79956d0a040fcaab15071da5beac474f7f(2026-04-26); last gooddef567677b876d32176b5e16648408c9f0fefabc(2026-04-24); range includes #29393 (ModuleLoader rewrite).Why
moduleRegistryModuleSettled(JSMicrotask.cpp:866) fires twice for the sameModuleRegistryEntry:hostLoadImportedModule's synchronous-replay path (JSModuleLoader.cpp:712-725, thefetchPromiseis Fulfilled branch) callsmakeModule+fetchComplete+modulePromise->fulfillPromiseinline while arequire(esm)drains the synchronous module queue. The entry'sm_statustransitionsFetching → Fetched.If a
ModuleRegistryFetchSettledreaction had already run on the normal microtask queue for that same entry before therequire(esm)entered sync mode, it left aModuleRegistryModuleSettledreaction queued there too. When the normal queue later drained, that stale reaction re-enteredfetchCompleteon the already-Fetchedentry and tripped the assertion.moduleRegistryFetchSettledalready guards this exact shape:moduleRegistryModuleSettleddid not.How (oven-sh/WebKit#217)
One
#if USE(BUN_JSC_ADDITIONS)guard at the top ofmoduleRegistryModuleSettled, matching the guard that already existed inmoduleRegistryFetchSettled. WhenmodulePromiseis no longerPending, the synchronous-replay path has already driven the entry throughfetchComplete+ fulfilledmodulePromiseinline, so there is nothing left for the stale normal-queue reaction to do. The secondAbstractModuleRecordproduced by the redundantmakeModulecall has no references and is GC-collected.This PR (the test side)
Same split pattern as #30186 / oven-sh/WebKit#214 — no
WEBKIT_VERSIONbump, just the regression test. The test installsreact@19+@mui/material@9+@emotion/styled@11+@emotion/react@11(emotion pinned to what MUI v9 peerDepends on, so a future emotion major can't reshape the graph) and runsbun -e 'require("./repro.js")'. It then runtime-probes the crash shape:134/133andASSERTION FAILED: m_status == Status::Fetchingon stderr → bug still present → log+skip, CI-green.0→ assertstdout === "loaded\n".That means CI passes on the currently-pinned WebKit (bug present → skip) and the test auto-lights-up the moment
WEBKIT_VERSIONinscripts/build/deps/webkit.tsis next bumped to a build containing oven-sh/WebKit#217. No coordination required between merging the two PRs.Verified locally:
avxsupport #217 applied: test runs the assertions and passes.