Upgrade WebKit to 87fd0daba19a (module-loader rewrite)#29393
Conversation
|
Updated 3:38 PM PT - Apr 25th, 2026
❌ @Jarred-Sumner, your commit 64af916 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29393That installs a local version of the PR into your bun-29393 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
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:
WalkthroughReplaces JSInternalPromise with JSPromise across bindings/C-ABI, migrates module loader from a JS Map to ModuleRegistryEntry/moduleLoader APIs, adds ESM registry intrinsics and a synchronous ESM loader, records top-level-await metadata, inserts module-loader JSType variants, and updates the WebKit prebuilt tag. Changes
Possibly related PRs
🚥 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/js/builtins/CommonJS.ts (1)
180-311: 🧹 Nitpick | 🔵 TrivialRemove the commented-out legacy loader body.
The short rationale is useful, but keeping the full pre-rewrite implementation inline makes this already-sensitive builtin harder to audit and guarantees drift from the real behavior. Please keep the note and delete the dead body, or move the historical context to the PR/docs instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/builtins/CommonJS.ts` around lines 180 - 311, Remove the large commented-out legacy implementation block for loadEsmIntoCjs__dead and leave only the short rationale note: delete the entire /* ... */ block that contains the old implementation and its internals but preserve a concise one- or two-line comment explaining the reason for removal; ensure no other code around the loadEsmIntoCjs__dead symbol is changed and that only the dead-body comment is removed.src/bun.js/bindings/BunPlugin.cpp (1)
630-669:⚠️ Potential issue | 🟠 MajorDon't preserve unevaluated ESM entries when installing a mock.
$esmNamespaceForCjs()only exposes a registry entry afterisModuleEvaluated(entry->record())succeeds, but this branch clearsremoveFromESMfor any entry that has a record and a namespace object. If the module is only fetched/linked, the mock stays hidden fromrequire(), and the later real evaluation can overwrite the mocked exports. Only keep the registry entry when the record is already evaluated; otherwise remove it and let the virtual-module path recreate the mock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/BunPlugin.cpp` around lines 630 - 669, The code clears removeFromESM for any registry entry with a record and namespace object, which preserves unevaluated ESM entries and hides mocks; update the branch that handles entry->record() (the block using moduleLoader()->registryEntry(specifierIdent), mod = entry->record(), moduleNamespaceObject = mod->getModuleNamespace(...), and overrideExportValue) to first check isModuleEvaluated(entry->record()) (or an equivalent isModuleEvaluated(mod)) and only set removeFromESM = false and populate the namespace when that evaluation check returns true; if the record is not evaluated, leave removeFromESM true and skip installing the mock into the registry so the virtual-module path can recreate it later.src/bun.js/bindings/bindings.cpp (2)
3077-3084:⚠️ Potential issue | 🔴 CriticalAdd null check before dereferencing
promisein the exception handler.Line 29-34 directly calls
promise->rejectWithCaughtException()without checking for null.JSModuleLoader__import()above (line 11) documents the contract viaEXCEPTION_ASSERT(!!scope.exception() == !promise): when a synchronous exception occurs,importModule()returnsnullptr. Since the scope is declared after theimportModule()call here (line 31), a synchronous failure will leavepromisenull whilescope.exception()is true, causing a null-pointer dereference on line 34.Guard with
if (promise && scope.exception())or declare the scope before the call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/bindings.cpp` around lines 3077 - 3084, The code may dereference a null promise when a synchronous exception occurs because DECLARE_THROW_SCOPE(vm) is declared after JSC::importModule; either move the DECLARE_THROW_SCOPE(vm) declaration to before the call to JSC::importModule(globalObject, ...) so scope.exception() is evaluated safely, or add a null check and change the condition to if (promise && scope.exception()) before calling promise->rejectWithCaughtException(globalObject, scope); update references to globalObject, promise, DECLARE_THROW_SCOPE, JSC::importModule, and rejectWithCaughtException accordingly.
3779-3804:⚠️ Potential issue | 🟠 MajorUse canonical
JSPromisehelpers after the alias switch.After aliasing
JSInternalPromisetoJSPromise,JSC__JSInternalPromise__result()andJSC__JSInternalPromise__setHandled()still use the old internal-promise implementations.result()lacks a pending-state guard (exposingreactionsOrResultfor pending promises), andsetHandled()manually flips flags instead of callingmarkAsHandled()(skipping rejection tracker bookkeeping).Align with the canonical implementations in this file:
Suggested fix
JSC::EncodedJSValue JSC__JSInternalPromise__result(const JSC::JSPromise* arg0) { - return JSC::JSValue::encode(arg0->result()); + switch (arg0->status()) { + case JSC::JSPromise::Status::Rejected: + case JSC::JSPromise::Status::Fulfilled: + return JSC::JSValue::encode(arg0->result()); + default: + return JSC::JSValue::encode(JSC::JSValue {}); + } } ... void JSC__JSInternalPromise__setHandled(JSC::JSPromise* promise, JSC::VM* arg1) { - auto& vm = *arg1; - auto flags = promise->internalField(JSC::JSPromise::Field::Flags).get().asUInt32(); - promise->internalField(JSC::JSPromise::Field::Flags).set(vm, promise, jsNumber(flags | JSC::JSPromise::isHandledFlag)); + (void)arg1; + promise->markAsHandled(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/bindings.cpp` around lines 3779 - 3804, Replace the manual/potentially-unsafe implementations with the canonical helpers: in JSC__JSInternalPromise__result(const JSC::JSPromise*), guard against Pending by checking arg0->status() == JSC::JSPromise::Status::Pending and return an encoded undefined (do not expose reactionsOrResult) otherwise return JSC::JSValue::encode(arg0->result()); in JSC__JSInternalPromise__setHandled(JSC::JSPromise* promise, JSC::VM* vm) stop manipulating internal flags directly and call the canonical promise method promise->markAsHandled(*vm) (or the exact markAsHandled signature used elsewhere) so rejection-tracker bookkeeping runs correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The WEBKIT_VERSION constant was set to a commit that has no
corresponding prebuilt release, causing prebuiltUrl() to fail; fix this by
either restoring WEBKIT_VERSION to a previously published commit hash (replace
the value in the exported WEBKIT_VERSION constant) or ensure the matching
prebuilt release is published under the autobuild-<commit> tag in oven-sh/WebKit
so prebuiltUrl() can fetch assets successfully; update WEBKIT_VERSION only after
verifying the autobuild-<hash> release exists.
In `@src/bake/BakeSourceProvider.cpp`:
- Around line 117-122: The code currently uses ASSERT(entry) and ASSERT(module)
which vanish in release builds and can let module be null when calling
global->moduleLoader()->getModuleNamespaceObject(...); replace these ASSERTs
with explicit null checks: after obtaining entry from
global->moduleLoader()->registryEntry(keyIdent) validate entry and if null
either throw a JS exception (via vm->throwException or appropriate JSC throw
helper) or return a sentinel/error value, then get module via entry->record(),
check module for null and likewise throw/return instead of proceeding to
getModuleNamespaceObject; update callers of
BakeLoadModuleByKey/BakeSourceProvider C ABI to handle the thrown exception or
sentinel result so loader failures do not crash.
In `@src/bun.js/bindings/InspectorLifecycleAgent.cpp`:
- Around line 153-155: The loop in InspectorLifecycleAgent.cpp exports ESM graph
items but only checks key.first, allowing null/stale ModuleRegistryEntry values
to leak; update the loop over global->moduleLoader()->moduleMap() to also skip
entries where entry is null (i.e., require entry to be non-null) before calling
esm->addItem(String{ key.first }) so it mirrors the guard used in
ZigGlobalObject.cpp.
In `@src/bun.js/bindings/JSType.zig`:
- Around line 208-221: JSType.isHidden() currently omits the five new
module-loader enum entries so they are treated as visible; update the isHidden()
implementation to treat ModuleRegistryEntry, ModuleLoadingContext,
ModuleLoaderPayload, ModuleGraphLoadingState, and JSModuleLoader as hidden
(either by adding explicit cases for those symbols or by extending the existing
hidden-range check) so any logic using JSType.isHidden() will hide these loader
bookkeeping types.
In `@src/bun.js/bindings/ModuleLoader.cpp`:
- Around line 478-488: The short-circuit that checks
registryEntry(specifierIdent) and returns when entry->status() >= Fetched leaves
the previously-created JSPromise* promise (from encodedPromiseValue) unsettled;
update the branch so it settles that promise before returning. Locate the
JSPromise* promise and encodedPromiseValue created earlier in ModuleLoader.cpp
and, in the if (entry->status() >= JSC::ModuleRegistryEntry::Status::Fetched)
block, resolve (or reject if appropriate) the promise using the same resolution
path used by the other exits so the fetch promise is always settled prior to the
return.
In `@src/bun.js/bindings/NodeVMSourceTextModule.cpp`:
- Around line 361-365: In NodeVMSourceTextModule::link, capture the
Synchronousness returned by record->link(globalObject, scriptFetcher) into a
variable (e.g. Synchronousness sync), call RETURN_IF_EXCEPTION(scope, {}), then
check if sync == Synchronousness::Async and assert/fail similarly to
NodeVMSyntheticModule::link (RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE or
equivalent) before calling status(Status::Linked); this ensures you do not set
the module to Linked when linking is async.
In `@src/bun.js/bindings/ZigGlobalObject.cpp`:
- Around line 682-706: The code unconditionally calls loader->removeEntry(key)
on the unsupported async path which can evict an entry that pre-existed (e.g.,
started by import()), causing cache misses; record whether an entry existed
before calling loader->loadModuleSync (use the earlier registryEntry lookup or a
bool preExisting) and only call loader->removeEntry(key) when this call actually
created the registry entry (i.e., !preExisting); apply the same guarded removal
change to the other occurrence mentioned (the block around the lines 720-722)
referencing moduleLoader(), registryEntry(), loadModuleSync(), removeEntry(),
and the throwVMTypeError unsupported-async path.
In `@src/js/builtins/BunBuiltinNames.h`:
- Around line 129-132: The builtin-name table must remain alphabetically sorted;
the four new symbols (esmNamespaceForCjs, esmRegistryDelete,
esmRegistryEvaluatedKeys, esmLoadSync) were appended after
fulfillModuleSync—move these four entries into the correct alphabetical location
among the other e* entries (i.e., with the existing "e..." block) so the list
stays sorted; update the placement near the other e-prefixed macros and leave
the rest of the table unchanged.
---
Outside diff comments:
In `@src/bun.js/bindings/bindings.cpp`:
- Around line 3077-3084: The code may dereference a null promise when a
synchronous exception occurs because DECLARE_THROW_SCOPE(vm) is declared after
JSC::importModule; either move the DECLARE_THROW_SCOPE(vm) declaration to before
the call to JSC::importModule(globalObject, ...) so scope.exception() is
evaluated safely, or add a null check and change the condition to if (promise &&
scope.exception()) before calling
promise->rejectWithCaughtException(globalObject, scope); update references to
globalObject, promise, DECLARE_THROW_SCOPE, JSC::importModule, and
rejectWithCaughtException accordingly.
- Around line 3779-3804: Replace the manual/potentially-unsafe implementations
with the canonical helpers: in JSC__JSInternalPromise__result(const
JSC::JSPromise*), guard against Pending by checking arg0->status() ==
JSC::JSPromise::Status::Pending and return an encoded undefined (do not expose
reactionsOrResult) otherwise return JSC::JSValue::encode(arg0->result()); in
JSC__JSInternalPromise__setHandled(JSC::JSPromise* promise, JSC::VM* vm) stop
manipulating internal flags directly and call the canonical promise method
promise->markAsHandled(*vm) (or the exact markAsHandled signature used
elsewhere) so rejection-tracker bookkeeping runs correctly.
In `@src/bun.js/bindings/BunPlugin.cpp`:
- Around line 630-669: The code clears removeFromESM for any registry entry with
a record and namespace object, which preserves unevaluated ESM entries and hides
mocks; update the branch that handles entry->record() (the block using
moduleLoader()->registryEntry(specifierIdent), mod = entry->record(),
moduleNamespaceObject = mod->getModuleNamespace(...), and overrideExportValue)
to first check isModuleEvaluated(entry->record()) (or an equivalent
isModuleEvaluated(mod)) and only set removeFromESM = false and populate the
namespace when that evaluation check returns true; if the record is not
evaluated, leave removeFromESM true and skip installing the mock into the
registry so the virtual-module path can recreate it later.
In `@src/js/builtins/CommonJS.ts`:
- Around line 180-311: Remove the large commented-out legacy implementation
block for loadEsmIntoCjs__dead and leave only the short rationale note: delete
the entire /* ... */ block that contains the old implementation and its
internals but preserve a concise one- or two-line comment explaining the reason
for removal; ensure no other code around the loadEsmIntoCjs__dead symbol is
changed and that only the dead-body comment is removed.
🪄 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: 6f6bfbdb-aeed-4de7-83c6-f5f9ed5ba481
📒 Files selected for processing (38)
scripts/build/deps/webkit.tssrc/ast/Macro.zigsrc/bake/BakeGlobalObject.cppsrc/bake/BakeSourceProvider.cppsrc/bake/production.zigsrc/bun.js.zigsrc/bun.js/AsyncModule.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/bindings/AnyPromise.zigsrc/bun.js/bindings/BunAnalyzeTranspiledModule.cppsrc/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/HTMLEntryPoint.cppsrc/bun.js/bindings/InspectorLifecycleAgent.cppsrc/bun.js/bindings/JSInternalPromise.zigsrc/bun.js/bindings/JSMockFunction.cppsrc/bun.js/bindings/JSNextTickQueue.cppsrc/bun.js/bindings/JSType.zigsrc/bun.js/bindings/ModuleLoader.cppsrc/bun.js/bindings/ModuleLoader.hsrc/bun.js/bindings/NodeVM.cppsrc/bun.js/bindings/NodeVM.hsrc/bun.js/bindings/NodeVMModule.cppsrc/bun.js/bindings/NodeVMSourceTextModule.cppsrc/bun.js/bindings/URLDecomposition.cppsrc/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/ZigGlobalObject.hsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/headers-cpp.hsrc/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/headers.hsrc/bun.js/bindings/napi.cppsrc/bun.js/bindings/webcore/Worker.cppsrc/bun.js/modules/NodeModuleModule.cppsrc/bun.js/web_worker.zigsrc/cli/test_command.zigsrc/js/builtins.d.tssrc/js/builtins/BunBuiltinNames.hsrc/js/builtins/CommonJS.ts
💤 Files with no reviewable changes (1)
- src/bun.js/bindings/HTMLEntryPoint.cpp
| auto keyIdent = JSC::Identifier::fromString(vm, key->value(global)); | ||
| auto* entry = global->moduleLoader()->registryEntry(keyIdent); | ||
| ASSERT(entry); // should have called BakeLoadServerCode and wait for that promise | ||
| auto* module = entry ? entry->record() : nullptr; | ||
| ASSERT(module); | ||
| JSC::JSModuleNamespaceObject* namespaceObject = global->moduleLoader()->getModuleNamespaceObject(global, module); |
There was a problem hiding this comment.
Don't rely on ASSERT for missing module records here.
BakeLoadModuleByKey() now returns a promise, so this C ABI can be reached before the load settles or after it fails. In release builds both asserts disappear, which means module can still be null when passed to getModuleNamespaceObject(...), turning a loader error into a crash. Please guard entry/entry->record() and throw or return a sentinel value instead of depending on debug-only checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bake/BakeSourceProvider.cpp` around lines 117 - 122, The code currently
uses ASSERT(entry) and ASSERT(module) which vanish in release builds and can let
module be null when calling
global->moduleLoader()->getModuleNamespaceObject(...); replace these ASSERTs
with explicit null checks: after obtaining entry from
global->moduleLoader()->registryEntry(keyIdent) validate entry and if null
either throw a JS exception (via vm->throwException or appropriate JSC throw
helper) or return a sentinel/error value, then get module via entry->record(),
check module for null and likewise throw/return instead of proceeding to
getModuleNamespaceObject; update callers of
BakeLoadModuleByKey/BakeSourceProvider C ABI to handle the thrown exception or
sentinel result so loader failures do not crash.
| /// Module loader registry entry (new C++ module loader). | ||
| ModuleRegistryEntry = 29, | ||
|
|
||
| /// Module loading context (new C++ module loader). | ||
| ModuleLoadingContext = 30, | ||
|
|
||
| /// Module loader payload (new C++ module loader). | ||
| ModuleLoaderPayload = 31, | ||
|
|
||
| /// Module graph loading state (new C++ module loader). | ||
| ModuleGraphLoadingState = 32, | ||
|
|
||
| /// JSModuleLoader cell type (new C++ module loader). | ||
| JSModuleLoader = 33, |
There was a problem hiding this comment.
Mark the new module-loader cells as hidden too.
These five additions are loader bookkeeping types, but JSType.isHidden() still only covers the older pre-Object internals. Any path that relies on isHidden() will now start treating ModuleRegistryEntry, ModuleLoadingContext, ModuleLoaderPayload, ModuleGraphLoadingState, and JSModuleLoader as ordinary visible runtime types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/JSType.zig` around lines 208 - 221, JSType.isHidden()
currently omits the five new module-loader enum entries so they are treated as
visible; update the isHidden() implementation to treat ModuleRegistryEntry,
ModuleLoadingContext, ModuleLoaderPayload, ModuleGraphLoadingState, and
JSModuleLoader as hidden (either by adding explicit cases for those symbols or
by extending the existing hidden-range check) so any logic using
JSType.isHidden() will hide these loader bookkeeping types.
| record->link(globalObject, scriptFetcher); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| if (sync == Synchronousness::Async) { | ||
| RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE("TODO(@heimskr): async SourceTextModule linking"); | ||
| } | ||
|
|
||
| status(Status::Linked); | ||
| return jsUndefined(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect current link() declarations/definitions and return types in JSC + Bun bindings.
rg -n -C3 --type=cpp --type=h '\blink\s*\(.*scriptFetcher|\blink\s*\(.*JSGlobalObject' \
vendor/WebKit/Source/JavaScriptCore src/bun.js/bindings
# 2) Compare SourceText vs Synthetic module handling around link() results.
rg -n -C6 --type=cpp 'NodeVMSourceTextModule::link|NodeVMSyntheticModule::link|Synchronousness|record->link' \
src/bun.js/bindings/NodeVMSourceTextModule.cpp src/bun.js/bindings/NodeVMSyntheticModule.cpp
# 3) Check whether async-link TODO/assert patterns still exist in related NodeVM codepaths.
rg -n -C3 --type=cpp 'TODO\(`@heimskr`\).*async|Synchronousness::Async|async .*link' src/bun.js/bindingsRepository: oven-sh/bun
Length of output: 4032
🏁 Script executed:
# 1) View the full NodeVMSourceTextModule::link implementation around lines 361-365
cat -n src/bun.js/bindings/NodeVMSourceTextModule.cpp | sed -n '315,380p'Repository: oven-sh/bun
Length of output: 3030
🏁 Script executed:
# 2) View the full NodeVMSyntheticModule::link implementation to see how Synchronousness is used
cat -n src/bun.js/bindings/NodeVMSyntheticModule.cpp | sed -n '130,180p'Repository: oven-sh/bun
Length of output: 2098
🏁 Script executed:
# 3) Find Synchronousness enum definition and understand its meaning
rg -n -B2 -A8 'enum.*Synchronousness|enum class Synchronousness' --type=h --type=cpp src/bun.js/bindings vendor/WebKit/Source/JavaScriptCoreRepository: oven-sh/bun
Length of output: 130
🏁 Script executed:
# Search for Synchronousness enum definition
rg -n 'enum.*Synchronousness|Synchronousness.*{' --type=h --type=cpp src/bun.js/bindingsRepository: oven-sh/bun
Length of output: 198
🏁 Script executed:
# Search for JSModuleRecord::link definition and return type
rg -n -B2 -A15 'JSModuleRecord::link' --type=h --type=cpp src/bun.js/bindingsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Search for any TODOs or comments about async linking in NodeVMSourceTextModule
rg -n 'TODO|FIXME|async|Async' src/bun.js/bindings/NodeVMSourceTextModule.cpp | head -30Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Simpler search for Synchronousness definition
rg -n 'Synchronousness' --type=h --type=cpp src/bun.js/bindings | head -20Repository: oven-sh/bun
Length of output: 354
🏁 Script executed:
# Look at the header file to understand JSModuleRecord::link
cat -n src/bun.js/bindings/NodeVMSourceTextModule.hRepository: oven-sh/bun
Length of output: 3981
🏁 Script executed:
# Search for JSModuleRecord in the codebase
rg -n 'class JSModuleRecord' --type=h --type=cpp src/bun.js/bindingsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check the include path for Synchronousness
grep -n 'Synchronousness' src/bun.js/bindings/JSCommonJSModule.cppRepository: oven-sh/bun
Length of output: 101
🏁 Script executed:
# Search for JSModuleRecord definition in the codebase
rg -n 'JSModuleRecord' --type=h src/bun.js/bindings | head -20Repository: oven-sh/bun
Length of output: 356
🏁 Script executed:
# Check if JSModuleRecord is used elsewhere and how link() is called
rg -n -B3 -A3 'JSModuleRecord.*link|->link\(' --type=cpp src/bun.js/bindings | grep -A3 -B3 'JSModuleRecord\|SourceText'Repository: oven-sh/bun
Length of output: 1699
🏁 Script executed:
# Look for any references to async in NodeVMSourceTextModule context
rg -n -B5 -A5 'record->link' src/bun.js/bindings/NodeVMSourceTextModule.cppRepository: oven-sh/bun
Length of output: 354
Capture and check Synchronousness return from record->link() before setting Linked status.
On Line 361, record->link(globalObject, scriptFetcher) returns a Synchronousness value that indicates whether linking is synchronous or async. The return value is currently discarded, and status is unconditionally set to Linked on Line 364.
The NodeVMSyntheticModule::link() implementation (lines 146–154) correctly captures this return value and explicitly asserts if async linking is encountered:
Synchronousness sync = record->link(globalObject, scriptFetcher);
RETURN_IF_EXCEPTION(scope, {});
if (sync == Synchronousness::Async) {
RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE("TODO(`@heimskr`): async SyntheticModule linking");
}
status(Status::Linked);The NodeVMSourceTextModule::link() must apply the same guard to avoid marking the module as Linked before async linking completes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/NodeVMSourceTextModule.cpp` around lines 361 - 365, In
NodeVMSourceTextModule::link, capture the Synchronousness returned by
record->link(globalObject, scriptFetcher) into a variable (e.g. Synchronousness
sync), call RETURN_IF_EXCEPTION(scope, {}), then check if sync ==
Synchronousness::Async and assert/fail similarly to NodeVMSyntheticModule::link
(RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE or equivalent) before calling
status(Status::Linked); this ensures you do not set the module to Linked when
linking is async.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| bool removeFromESM = false; | ||
| bool removeFromCJS = false; | ||
|
|
||
| JSValue entryValue = esm->get(globalObject, specifierString); | ||
| auto specifierIdent = JSC::Identifier::fromString(vm, specifierString->value(globalObject)); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| if (entryValue) { | ||
| if (auto* entry = globalObject->moduleLoader()->registryEntry(specifierIdent)) { | ||
| removeFromESM = true; | ||
| JSObject* entry = entryValue ? entryValue.getObject() : nullptr; | ||
| if (entry) { | ||
| auto moduleValue = entry->getIfPropertyExists(globalObject, Identifier::fromString(vm, String("module"_s))); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| if (moduleValue) { | ||
| if (auto* mod = jsDynamicCast<JSC::AbstractModuleRecord*>(moduleValue)) { | ||
| if (auto* mod = entry->record()) { | ||
| { | ||
| { | ||
| JSC::JSModuleNamespaceObject* moduleNamespaceObject = mod->getModuleNamespace(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| if (moduleNamespaceObject) { |
There was a problem hiding this comment.
🟡 The refactoring of JSMock__jsModuleMock in BunPlugin.cpp left behind two empty brace pairs at lines 633–634. The code now reads if (auto* mod = entry->record()) { { { where the inner { { are dead nesting artifacts from the removed intermediate if (entry) and if (moduleValue) checks — they should be deleted.
Extended reasoning...
What the bug is
During the migration from the old JSMap-based ESM registry to the new C++ ModuleRegistryEntry API, the JSMock__jsModuleMock function was refactored to collapse four nested if-conditions into two. The old code had: if (entryValue) { JSObject* entry = ...; if (entry) { auto moduleValue = entry->getIfPropertyExists(...); RETURN_IF_EXCEPTION(...); if (moduleValue) { if (auto* mod = jsDynamicCast<JSC::AbstractModuleRecord*>(moduleValue)) {. The new code correctly uses if (auto* entry = globalObject->moduleLoader()->registryEntry(specifierIdent)) and if (auto* mod = entry->record()), but the two intermediate opening braces from the now-deleted if (entry) { and if (moduleValue) { blocks were accidentally left in place.
The specific code path
At src/bun.js/bindings/BunPlugin.cpp, after the refactor, the code reads:
if (auto* mod = entry->record()) {
{
{
JSC::JSModuleNamespaceObject* moduleNamespaceObject = mod->getModuleNamespace(globalObject);The two innermost { have no corresponding if-condition and enclose no scope-specific declarations; they are purely orphaned brace pairs.
Why existing code doesn't prevent it
C++ treats bare braces as valid (but no-op) scoping blocks. The compiler accepts this without warning, so nothing catches the dead nesting automatically.
Impact
No functional impact — the extra braces do not change control flow or scope any variables. The sole effect is unnecessary indentation and reduced readability, making the code harder to understand and review.
How to fix
Delete the two orphaned open/close brace pairs so the block reads:
if (auto* mod = entry->record()) {
JSC::JSModuleNamespaceObject* moduleNamespaceObject = mod->getModuleNamespace(globalObject);
...
}Step-by-step proof
- Old code:
if (entryValue) { // brace A,if (entry) { // brace B,if (moduleValue) { // brace C,if (auto* mod = ...) { // brace D - New code removes all outer conditions, keeping only
if (auto* entry = ...) {andif (auto* mod = entry->record()) {. - However, the bodies of the removed conditions (brace B and brace C) were not removed — their opening braces remain as bare
{tokens at the start of the newif (auto* mod)body. - Inspecting the diff confirms lines
+ {and+ {appear immediately insideif (auto* mod = entry->record()) {with no associated condition, and are closed again at the end of the block, matching the old closing braces of the removedif (entry)andif (moduleValue)conditions.
There was a problem hiding this comment.
One of the two orphaned brace pairs was addressed (it became the if (linked) { condition), but one bare { … } pair still remains at lines 643/679 — immediately inside if (linked) {. It encloses no scope-specific declarations and can be deleted so the block reads:
if (linked) {
JSC::JSModuleNamespaceObject* moduleNamespaceObject = mod->getModuleNamespace(globalObject);
...
}| scope.release(); | ||
| } | ||
|
|
||
| JSC::JSInternalPromise* JSC__JSValue__asInternalPromise(JSC::EncodedJSValue JSValue0) | ||
| JSC::JSPromise* JSC__JSValue__asInternalPromise(JSC::EncodedJSValue JSValue0) | ||
| { | ||
| JSC::JSValue value = JSC::JSValue::decode(JSValue0); | ||
| return JSC::jsDynamicCast<JSC::JSInternalPromise*>(value); | ||
| return JSC::jsDynamicCast<JSC::JSPromise*>(value); | ||
| } | ||
|
|
||
| JSC::JSPromise* JSC__JSValue__asPromise(JSC::EncodedJSValue JSValue0) |
There was a problem hiding this comment.
🟡 The .normal branch in asAnyPromise() (JSValue.zig:659-663) is now permanently dead code since this PR aliased JSInternalPromise to JSPromise. Both JSC__JSValue__asInternalPromise and JSC__JSValue__asPromise in bindings.cpp now perform the identical jsDynamicCast<JSC::JSPromise*>(value) cast, so asInternalPromise() always succeeds first and the .normal path is unreachable. No behavioral impact today because all AnyPromise methods use inline else => treating both tags identically, but the union distinction is now semantically meaningless.
Extended reasoning...
What the bug is:
After the JSInternalPromise removal (d36f217d6899), JSInternalPromise was made a transparent alias for JSPromise in JSInternalPromise.zig. Both C++ bindings that back the Zig FFI were updated to perform identical casts in bindings.cpp (lines 2657-2666):
- JSC__JSValue__asInternalPromise -> jsDynamicCastJSC::JSPromise*(value)
- JSC__JSValue__asPromise -> jsDynamicCastJSC::JSPromise*(value)
The specific code path:
asAnyPromise() in JSValue.zig (lines 650-664) tries the two branches sequentially: first calling asInternalPromise() at line 654, then asPromise() at line 659. Before this PR, asInternalPromise() would only succeed for JSInternalPromise objects (a distinct subclass with a different ClassInfo), so asPromise() could independently succeed for regular JSPromise values. After this PR, both functions do the same cast, so any JSPromise always matches the .internal branch first.
Why existing code does not prevent it:
The asAnyPromise() function has no guard preventing this ordering issue. The .internal check is unconditionally tried first, and since it now succeeds for all promises (not just internal ones), the .normal check at lines 659-663 is unreachable.
Step-by-step proof:
Take a regular new Promise((resolve, reject) => {}) value and call jsValue.asAnyPromise():
- asInternalPromise() calls JSC__JSValue__asInternalPromise -> jsDynamicCastJSC::JSPromise*(value) -> succeeds, returns non-null pointer
- asAnyPromise() immediately returns AnyPromise{ .internal = promise }
- Line 659 (if (value.asPromise())) is never evaluated
So the .normal branch can never be returned from asAnyPromise() after this PR.
Impact today:
No runtime bug. Since JSInternalPromise = JSPromise (they are now the same type), AnyPromise.internal and AnyPromise.normal both hold a *JSPromise, and every AnyPromise method uses inline else => which handles both tags identically. Callers checking for .normal from asAnyPromise() will never observe it.
How to fix:
The simplest fix is to collapse the AnyPromise union to a single variant now that the distinction is gone, or remove the .normal branch from asAnyPromise() and swap in asPromise() only. The two-variant union is now dead weight.
|
|
||
| auto* entry = loader->registryEntry(key); | ||
| if (!entry || !entry->record()) [[unlikely]] | ||
| return throwVMTypeError(globalObject, scope, makeString("require() failed to evaluate module \""_s, keyString, "\". This is an internal consistentency error."_s)); |
There was a problem hiding this comment.
🟡 The newly introduced functionEsmLoadSync in ZigGlobalObject.cpp (line 712) contains a typo: 'consistentency' (extra 'en') instead of 'consistency' in the error message. Fix by changing the string to read: "This is an internal consistency error."
Extended reasoning...
What the bug is: The newly added functionEsmLoadSync C++ function in ZigGlobalObject.cpp contains a misspelled word in an error message at line ~712. The string reads "This is an internal consistentency error." where 'consistentency' has an extra 'en' — the correct spelling is 'consistency'.
The specific code path: The error message is the fallback case in functionEsmLoadSync when the module registry entry or record cannot be found after a successful loadModuleSync call:
if (\!entry || \!entry->record()) [[unlikely]]
return throwVMTypeError(globalObject, scope, makeString("require() failed to evaluate module ..." "This is an internal consistentency error."));
Why existing code does not prevent it: This is a new error string literal — there is no spell-checker or linting step in the build pipeline that catches typos in C++ string literals.
Impact: The typo surfaces in the user-facing error message when require() fails to evaluate an ESM module in an internal error edge case. While functionally harmless, it looks unprofessional and could confuse users searching for this exact error text online.
How to fix: Change 'consistentency' to 'consistency' in the makeString call at ZigGlobalObject.cpp line ~712.
Step-by-step proof: (1) A user calls require() on an ESM module. (2) functionEsmLoadSync is invoked, calls loadModuleSync, and the promise fulfills successfully. (3) The code then calls loader->registryEntry(key) to retrieve the loaded entry. (4) In the unlikely branch where entry is null or entry->record() is null, throwVMTypeError is called with the misspelled string. (5) The user sees: 'require() failed to evaluate module "foo". This is an internal consistentency error.' with the double 'en' typo.
Note: The same typo also exists in the pre-existing CommonJS.ts requireESM function, but that code predates this PR. Only the C++ instance is newly introduced by this PR.
|
yooo |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bun.js/bindings/bindings.cpp (2)
3779-3805:⚠️ Potential issue | 🟠 MajorKeep the
JSInternalPromisealiases behaviorally identical toJSPromise.
JSC__JSPromise__result()marks rejected promises as handled before exposing the result, butJSC__JSInternalPromise__result()now just returnsarg0->result(), andJSC__JSInternalPromise__setHandled()bypasses the canonicalmarkAsHandled()helper. Becausesrc/bun.js/bindings/JSModuleLoader.zig:37-45still returns*JSInternalPromise, existing Zig call sites will keep flowing through this alias path and can regress into spurious unhandled-rejection bookkeeping for rejected module-load promises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/bindings.cpp` around lines 3779 - 3805, The JSInternalPromise aliases must behave like JSPromise: update JSC__JSInternalPromise__result to call the same handling path as JSC__JSPromise__result (mark rejected promises handled before returning the result) instead of directly returning arg0->result(), and change JSC__JSInternalPromise__setHandled to use the canonical markAsHandled()/markHandled helper used by JSPromise (or invoke the same internal helper used by JSC__JSPromise__setHandled) rather than manipulating the Flags internalField manually; modify the implementations of JSC__JSInternalPromise__result and JSC__JSInternalPromise__setHandled accordingly to reuse the existing JSPromise handling helpers so behavior is identical.
3077-3095:⚠️ Potential issue | 🔴 CriticalGuard the
importModule()failure path before dereferencingpromise.
JSModuleLoader__import()in this file already assumes the new contract thatimportModule()returnsnullptrwhen it throws. This path still unconditionally callspromise->rejectWithCaughtException(...)andpromise->status(), so a resolution/parse error turns into a null-pointer crash instead of being surfaced througharg6.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/bindings.cpp` around lines 3077 - 3095, The code dereferences promise returned by JSC::importModule() without checking for nullptr, causing a crash when importModule throws; after calling JSC::importModule(...) in this block (and after DECLARE_THROW_SCOPE(vm)), check whether promise is nullptr before using it: if promise is nullptr and scope.exception() is set, encode the caught exception into *arg6 (same way the Rejected branch does) and return encoded jsUndefined, otherwise proceed to call promise->rejectWithCaughtException(...) and use promise->status(); ensure no calls to promise->... occur when promise == nullptr.src/bun.js/bindings/ZigGlobalObject.cpp (1)
3456-3459:⚠️ Potential issue | 🟠 MajorClear exception before rejecting the fetch promise.
moduleLoaderFetch()callsrejectedInternalPromise(globalObject, scope.exception()->value())at lines 3458, 3505, and 3521, extracting the exception value without clearing it from the ThrowScope. This leaves a pending exception when the function returns, violating the JSC contract.Compare to
moduleLoaderImportModule()(lines 3325, 3378, 3423), which correctly usesrejectedPromiseWithCaughtException(globalObject, scope)to handle exception clearing as part of promise construction.Use
rejectedPromiseWithCaughtException()instead of manually extracting and passingscope.exception()->value(), or explicitly callscope.clearException()before returning if usingrejectedInternalPromise().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/ZigGlobalObject.cpp` around lines 3456 - 3459, moduleLoaderFetch() currently returns rejectedInternalPromise(globalObject, scope.exception()->value()) while leaving the ThrowScope exception uncleared; update the function to either use rejectedPromiseWithCaughtException(globalObject, scope) (preferred) where you see those three returns, or if you must keep rejectedInternalPromise(), call scope.clearException() before returning so the ThrowScope has no pending exception. Locate the three occurrences in moduleLoaderFetch() and replace the manual extraction of scope.exception()->value() with rejectedPromiseWithCaughtException(globalObject, scope) or clear the exception via scope.clearException() immediately before calling rejectedInternalPromise().
♻️ Duplicate comments (3)
src/bun.js/bindings/NodeVMSourceTextModule.cpp (1)
372-385:⚠️ Potential issue | 🟠 MajorOnly mark the wrapper
Linkedafterrecord->link()completes synchronously.
link()now flipsstatus(Status::Linked)beforeinstantiate()runs the real JSC link step. Ifrecord->link()throws, or returnsSynchronousness::Async, this wrapper still looks linked andNodeVMModule::evaluateDependencies()can treat it as ready for evaluation.NodeVMSyntheticModule::link()already guards the async case; this path should do the same and move the status transition after a successful synchronous link.♻️ Suggested direction
- UNUSED_PARAM(scriptFetcher); - status(Status::Linked); + UNUSED_PARAM(scriptFetcher); return jsUndefined(); } JSValue NodeVMSourceTextModule::instantiate(JSGlobalObject* globalObject) { @@ - record->link(globalObject, jsUndefined()); + Synchronousness sync = record->link(globalObject, jsUndefined()); RETURN_IF_EXCEPTION(scope, {}); + if (sync == Synchronousness::Async) + RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE("TODO(`@heimskr`): async SourceTextModule linking"); + + status(Status::Linked); return jsUndefined(); }Run this to confirm the current ordering and the downstream status dependency:
#!/bin/bash set -euo pipefail echo "NodeVMSourceTextModule status transition vs record->link:" rg -n -C3 'status\(Status::Linked\)|record->link\(' src/bun.js/bindings/NodeVMSourceTextModule.cpp echo echo "NodeVMModule consumers that gate evaluation on wrapper status:" rg -n -C3 'status\(\) == Status::Unlinked|status\(\) == Status::Linked' src/bun.js/bindings/NodeVMModule.cppAlso applies to: 388-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/NodeVMSourceTextModule.cpp` around lines 372 - 385, The wrapper sets status(Status::Linked) before calling record->link(), which can throw or return Synchronousness::Async and leave the wrapper incorrectly marked linked; change NodeVMSourceTextModule's flow so you call record->link(), check that it returned Synchronousness::Sync and did not throw, and only then set status(Status::Linked) (mirroring NodeVMSyntheticModule::link() behavior); if record->link() throws, propagate the exception and do not set Linked, and if it returns Async, do not set Linked and let the async path handle the status transition later.src/js/builtins/BunBuiltinNames.h (1)
130-133: 🛠️ Refactor suggestion | 🟠 MajorKeep the builtin-name table sorted.
The list header (line 23) requires alphabetical order. The four new
esm*identifiers should be placed aftererrorStepsand beforeevaluateCommonJSModule, not afterfulfillModuleSync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/builtins/BunBuiltinNames.h` around lines 130 - 133, The four new builtin names (esmNamespaceForCjs, esmRegistryDelete, esmRegistryEvaluatedKeys, esmLoadSync) are out of alphabetical order; move their macro(...) entries so they appear after errorSteps and before evaluateCommonJSModule (i.e., insert them in the sorted position rather than after fulfillModuleSync) to keep the builtin-name table alphabetically ordered as required by the list header. Ensure you update only the ordering of these identifiers in BunBuiltinNames.h so the table remains strictly sorted.src/bun.js/bindings/InspectorLifecycleAgent.cpp (1)
154-162:⚠️ Potential issue | 🟡 MinorGuard against null registry entries.
The loop checks
key.firstbut notentry. Stale/deleted registry slots can have a valid key with a null entry. This mirrors the guard inZigGlobalObject.cpp:656-661.Suggested fix
for (auto& [key, entry] : global->moduleLoader()->moduleMap()) { - if (key.first) + if (key.first && entry) keys.append(String { key.first }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/InspectorLifecycleAgent.cpp` around lines 154 - 162, The loop that collects module keys from global->moduleLoader()->moduleMap() currently only checks key.first but not whether the associated entry is non-null, so update the loop in InspectorLifecycleAgent.cpp to skip entries where entry is null (mirror the null-entry guard used in ZigGlobalObject.cpp around the module map iteration); specifically, inside the for (auto& [key, entry] : global->moduleLoader()->moduleMap()) block ensure you check that entry is truthy before appending key.first to the keys Vector<String>, then continue to sort and call esm->addItem(k) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/BunAnalyzeTranspiledModule.cpp`:
- Around line 72-76: The debug parity check misses the new observable bit hasTLA
on JSModuleRecord, so discrepancies between the fast and fallback parsers can go
undetected; update the fallbackParse/dumpRecordInfo parity path to include
hasTLA in its dumped/compared fields (where dumpRecordInfo() is used to compare
fast vs fallback parses) and ensure JSC_JSModuleRecord__create / JSModuleRecord
exposes/reads the same hasTLA state for parity checks so the debug comparison
considers result->hasTLA (and any accessor like hasTLA()) when building the dump
and when comparing records.
In `@src/bun.js/bindings/ModuleLoader.cpp`:
- Around line 694-700: Extract the repeated pattern that creates and links a
JSC::VM::SynchronousModuleQueue into an RAII guard class (e.g.,
SynchronousModuleQueueScope) that in its constructor saves
vm.m_synchronousModuleQueue to its m_queue.prev and sets
vm.m_synchronousModuleQueue = &m_queue, and in its destructor restores
vm.m_synchronousModuleQueue = m_queue.prev; replace the four occurrences in
ModuleLoader.cpp (the blocks that construct a local
JSC::VM::SynchronousModuleQueue, set prev, assign vm.m_synchronousModuleQueue,
perform module operations, and restore prev) with a scoped instance of this
guard so restoration happens automatically even on early returns or exceptions,
preserving existing behavior and using the same JSC::VM::SynchronousModuleQueue
type and vm member names.
In `@src/bundler/linker_context/postProcessJSChunk.zig`:
- Around line 141-149: The code sets mi.flags.has_tla using the source-file
level array tla_keywords, which marks the whole chunk async even if the part
with the TLA was tree-shaken; instead, derive TLA presence from the live part
itself. In the loop over chunk.content.javascript.parts_in_chunk_in_order (using
part_range and idx via part_range.source_index), consult the corresponding
part's live metadata or printed output for a TLA marker (e.g., inspect the
part_range or the part's own AST/emit flags) rather than tla_keywords[idx] from
the whole-source AST; only set mi.flags.has_tla when that specific live part
actually contains a top-level await and skip parts that were removed by
tree-shaking (respect wraps[idx] as before).
In `@src/js/builtins/CommonJS.ts`:
- Around line 314-323: In requireESM (function requireESM) the thrown TypeError
message contains a typo "consistentency" — change it to "consistency" so the
message reads: `require() failed to evaluate module "${resolved}". This is an
internal consistency error.`; also update the same typo in the C++ equivalent
(functionEsmLoadSync in ZigGlobalObject.cpp) to keep messages consistent across
implementations.
In `@test/js/bun/net/socket.test.ts`:
- Around line 777-783: The comment explains that Windows pins an extra TCPSocket
but does not justify widening TLSSocket; update the FIXME and test assertions to
be explicit: either add a brief explanatory note in the comment that TLSSocket
is also observed pinned on Windows (so both await
expectMaxObjectTypeCount(expect, "TCPSocket", isWindows ? 4 : 2) and await
expectMaxObjectTypeCount(expect, "TLSSocket", isWindows ? 4 : 2) are
intentional), or revert TLSSocket to the strict count (await
expectMaxObjectTypeCount(expect, "TLSSocket", 2)) if TLSSocket is not actually
affected; locate the assertions by the function name expectMaxObjectTypeCount
and the type strings "TCPSocket" and "TLSSocket" and update the comment and
counts accordingly so the test explicitly documents the rationale for each
socket type on Windows via isWindows.
In `@test/js/bun/resolve/concurrent-dynamic-import.test.ts`:
- Around line 28-30: Remove the strict stderr emptiness check and instead assert
on stdout and the process exit code: delete the line expecting stderr to be
exactly "" (the variable from Promise.all: stderr) and add/ensure an assertion
that exitCode is 0 (the exitCode from Promise.all / proc.exited) while keeping
the stdout.trim() === "ok" check; this avoids ASAN noise from failing the test
when using bunExe() + bunEnv while keeping stdout and exitCode as the regression
guard.
In `@test/js/bun/resolve/dynamic-import-tla-cycle.test.ts`:
- Around line 33-37: The test currently asserts stderr is empty but ASAN startup
noise can appear; after collecting proc.stdout.text(), proc.stderr.text(), and
proc.exited, filter known ASAN lines from the stderr string (e.g., strip lines
matching /^==\d+==.*$/ or other AddressSanitizer startup messages) into a new
variable (e.g., filteredStderr) and assert that filteredStderr is empty instead
of raw stderr; update the expectations around proc, stdout, stderr and exitCode
(keep the existing checks for stdout.trim() and exitCode) so the test ignores
ASAN noise before asserting an empty stderr.
In `@test/js/bun/resolve/require-esm-gc-roots.test.ts`:
- Around line 58-61: The test currently asserts stderr is empty but ASAN startup
warnings from the spawned subprocess can cause flakes; before asserting on
stderr (the variable produced from proc.stderr.text()), filter out the known
ASAN startup noise (e.g. remove lines matching /WARNING: ASAN.*JSC signal
handlers.*\n?/ or a broader /WARNING: ASAN.*/ pattern) and trim the result, then
assert the cleaned stderr is empty; update the block that reads
proc.stdout.text(), proc.stderr.text(), proc.exited and replace the direct
expect(stderr).toBe("") with an assertion on the filtered/trimmed stderr string.
In `@test/js/bun/resolve/require-esm-microtask-order.test.ts`:
- Around line 40-42: The test currently asserts stderr is exactly empty but can
fail due to ASAN startup banner emitted by the subprocess; before the expect on
`stderr` (after awaiting
`proc.stdout.text()`/`proc.stderr.text()`/`proc.exited`), pass `stderr` through
the repo’s subprocess-stderr filter (the shared ASAN-startup filter helper used
across tests, e.g. filterSubprocessStderr or equivalent) to strip the known
`WARNING: ASAN interferes...` line, then assert the filtered result is empty;
reference `proc`, `stdout`, and `stderr` from this test and use the existing
filter helper rather than changing environment setup.
In `@test/js/bun/resolve/require-esm-transitive-tla.test.ts`:
- Around line 60-67: Replace the flaky setTimeout wait with a deterministic
readiness check: have the async module (side.mjs) expose a readiness signal
(e.g., export a Promise or boolean that is resolved/set when it reaches the
EvaluatingAsync/TLA-started point) and in the test await that signal after
starting the top-level import (the import("./side.mjs") call) before calling
require("./side.mjs"); this ensures the test only proceeds once the module is in
the expected EvaluatingAsync state rather than relying on a timing guess.
- Around line 44-47: The test currently asserts raw stderr is empty which is
brittle on ASAN/debug lanes; after awaiting proc.stderr.text() (the variable
stderr from the Promise.all that also produces stdout and exitCode), pass stderr
through the repo's subprocess-stderr filter function used in other bun tests
(e.g., the conventional filter that strips ASAN startup noise) and assert the
filtered result is empty instead of the raw stderr; apply the same change for
the other occurrence around lines 84-87 where stderr is asserted empty.
In `@test/js/web/fetch/fetch-leak-test-fixture-5.js`:
- Around line 11-15: Currently promiseThreshold is set by hardcoding "+ 10",
which bakes loader internals into the test; instead capture the initial Promise
count once and use that as the baseline. Replace the hardcoded const
promiseThreshold = threshold + 10; by recording the initial Promise count (e.g.,
capture a baselinePromises count before test iterations) and compute the allowed
limit as baselinePromises + threshold wherever promiseThreshold is used; update
references to use this computed baseline + threshold so the test adapts to
current loader behavior rather than assuming "+ 10".
---
Outside diff comments:
In `@src/bun.js/bindings/bindings.cpp`:
- Around line 3779-3805: The JSInternalPromise aliases must behave like
JSPromise: update JSC__JSInternalPromise__result to call the same handling path
as JSC__JSPromise__result (mark rejected promises handled before returning the
result) instead of directly returning arg0->result(), and change
JSC__JSInternalPromise__setHandled to use the canonical
markAsHandled()/markHandled helper used by JSPromise (or invoke the same
internal helper used by JSC__JSPromise__setHandled) rather than manipulating the
Flags internalField manually; modify the implementations of
JSC__JSInternalPromise__result and JSC__JSInternalPromise__setHandled
accordingly to reuse the existing JSPromise handling helpers so behavior is
identical.
- Around line 3077-3095: The code dereferences promise returned by
JSC::importModule() without checking for nullptr, causing a crash when
importModule throws; after calling JSC::importModule(...) in this block (and
after DECLARE_THROW_SCOPE(vm)), check whether promise is nullptr before using
it: if promise is nullptr and scope.exception() is set, encode the caught
exception into *arg6 (same way the Rejected branch does) and return encoded
jsUndefined, otherwise proceed to call promise->rejectWithCaughtException(...)
and use promise->status(); ensure no calls to promise->... occur when promise ==
nullptr.
In `@src/bun.js/bindings/ZigGlobalObject.cpp`:
- Around line 3456-3459: moduleLoaderFetch() currently returns
rejectedInternalPromise(globalObject, scope.exception()->value()) while leaving
the ThrowScope exception uncleared; update the function to either use
rejectedPromiseWithCaughtException(globalObject, scope) (preferred) where you
see those three returns, or if you must keep rejectedInternalPromise(), call
scope.clearException() before returning so the ThrowScope has no pending
exception. Locate the three occurrences in moduleLoaderFetch() and replace the
manual extraction of scope.exception()->value() with
rejectedPromiseWithCaughtException(globalObject, scope) or clear the exception
via scope.clearException() immediately before calling rejectedInternalPromise().
---
Duplicate comments:
In `@src/bun.js/bindings/InspectorLifecycleAgent.cpp`:
- Around line 154-162: The loop that collects module keys from
global->moduleLoader()->moduleMap() currently only checks key.first but not
whether the associated entry is non-null, so update the loop in
InspectorLifecycleAgent.cpp to skip entries where entry is null (mirror the
null-entry guard used in ZigGlobalObject.cpp around the module map iteration);
specifically, inside the for (auto& [key, entry] :
global->moduleLoader()->moduleMap()) block ensure you check that entry is truthy
before appending key.first to the keys Vector<String>, then continue to sort and
call esm->addItem(k) as before.
In `@src/bun.js/bindings/NodeVMSourceTextModule.cpp`:
- Around line 372-385: The wrapper sets status(Status::Linked) before calling
record->link(), which can throw or return Synchronousness::Async and leave the
wrapper incorrectly marked linked; change NodeVMSourceTextModule's flow so you
call record->link(), check that it returned Synchronousness::Sync and did not
throw, and only then set status(Status::Linked) (mirroring
NodeVMSyntheticModule::link() behavior); if record->link() throws, propagate the
exception and do not set Linked, and if it returns Async, do not set Linked and
let the async path handle the status transition later.
In `@src/js/builtins/BunBuiltinNames.h`:
- Around line 130-133: The four new builtin names (esmNamespaceForCjs,
esmRegistryDelete, esmRegistryEvaluatedKeys, esmLoadSync) are out of
alphabetical order; move their macro(...) entries so they appear after
errorSteps and before evaluateCommonJSModule (i.e., insert them in the sorted
position rather than after fulfillModuleSync) to keep the builtin-name table
alphabetically ordered as required by the list header. Ensure you update only
the ordering of these identifiers in BunBuiltinNames.h so the table remains
strictly sorted.
🪄 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: 448eae7f-8999-403c-8853-79780925d883
📒 Files selected for processing (47)
scripts/build/deps/webkit.tsscripts/verify-baseline-static/allowlist-x64-windows.txtscripts/verify-baseline-static/allowlist-x64.txtsrc/analyze_transpiled_module.zigsrc/bake/production.zigsrc/bun.js.zigsrc/bun.js/ModuleLoader.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/bindings/BunAnalyzeTranspiledModule.cppsrc/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/InspectorLifecycleAgent.cppsrc/bun.js/bindings/ModuleLoader.cppsrc/bun.js/bindings/ModuleLoader.hsrc/bun.js/bindings/NodeVMSourceTextModule.cppsrc/bun.js/bindings/ZigGlobalObject.cppsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/webcore/SerializedScriptValue.hsrc/bun.js/bindings/webcore/Worker.cppsrc/bun.js/modules/NodeModuleModule.cppsrc/bun.js/web_worker.zigsrc/bundler/linker_context/postProcessJSChunk.zigsrc/cli/test_command.zigsrc/js/builtins.d.tssrc/js/builtins/BunBuiltinNames.hsrc/js/builtins/CommonJS.tssrc/js/builtins/ReadableStreamInternals.tssrc/js/node/vm.tstest/bundler/bundler_splitting.test.tstest/cli/inspect/BunFrontendDevServer.test.tstest/cli/run/require-cache-bug-5188.jstest/cli/run/syntax.test.tstest/js/bun/net/socket.test.tstest/js/bun/plugin/plugin-namespace-drive-letter.test.tstest/js/bun/resolve/concurrent-dynamic-import.test.tstest/js/bun/resolve/dynamic-import-tla-cycle.test.tstest/js/bun/resolve/esModule.test.tstest/js/bun/resolve/import-query.test.tstest/js/bun/resolve/load-file-loader-a-lot.test.tstest/js/bun/resolve/require-esm-gc-roots.test.tstest/js/bun/resolve/require-esm-microtask-order.test.tstest/js/bun/resolve/require-esm-transitive-tla.test.tstest/js/bun/resolve/resolve-test.jstest/js/bun/util/reportError.test.tstest/js/web/console/console-log.test.tstest/js/web/fetch/fetch-leak-test-fixture-5.jstest/regression/issue/12910/t.mjs
💤 Files with no reviewable changes (2)
- test/js/bun/util/reportError.test.ts
- test/js/web/console/console-log.test.ts
| export function requireESM(this, resolved: string) { | ||
| var entry = Loader.registry.$get(resolved); | ||
|
|
||
| if (!entry || !entry.evaluated) { | ||
| entry = $loadEsmIntoCjs(resolved); | ||
| var exports = $esmNamespaceForCjs(resolved); | ||
| if (exports === undefined) { | ||
| exports = $loadEsmIntoCjs(resolved); | ||
| } | ||
|
|
||
| if (!entry || !entry.evaluated || !entry.module) { | ||
| if (exports === undefined) { | ||
| throw new TypeError(`require() failed to evaluate module "${resolved}". This is an internal consistentency error.`); | ||
| } | ||
| var exports = Loader.getModuleNamespaceObject(entry.module); | ||
|
|
||
| return exports; | ||
| } |
There was a problem hiding this comment.
Typo in error message: "consistentency" → "consistency".
✏️ Proposed fix
- throw new TypeError(`require() failed to evaluate module "${resolved}". This is an internal consistentency error.`);
+ throw new TypeError(`require() failed to evaluate module "${resolved}". This is an internal consistency error.`);Note: The same typo exists in the C++ side (functionEsmLoadSync in ZigGlobalObject.cpp around line 780). Consider fixing both for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/builtins/CommonJS.ts` around lines 314 - 323, In requireESM (function
requireESM) the thrown TypeError message contains a typo "consistentency" —
change it to "consistency" so the message reads: `require() failed to evaluate
module "${resolved}". This is an internal consistency error.`; also update the
same typo in the C++ equivalent (functionEsmLoadSync in ZigGlobalObject.cpp) to
keep messages consistent across implementations.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(stdout.trim()).toBe("ok"); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
Filter ASAN noise before asserting stderr is empty.
In debug ASAN builds, spawned subprocesses emit WARNING: ASAN interferes with JSC signal handlers… to stderr, which isn't suppressed by bunEnv. Use the repo's established convention to avoid flaky CI failures:
Suggested fix
- expect(stderr).toBe("");
+ expect(
+ stderr
+ .split("\n")
+ .filter((line) => !line.startsWith("WARNING: ASAN interferes"))
+ .join("\n")
+ ).toBe("");Based on learnings: "In oven-sh/bun test files that spawn subprocesses using bunEnv, suppress the known ASAN startup noise in the subprocess stderr before asserting it is empty."
📝 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.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(stderr).toBe(""); | |
| expect(stdout.trim()).toBe("ok"); | |
| expect(exitCode).toBe(0); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect( | |
| stderr | |
| .split("\n") | |
| .filter((line) => !line.startsWith("WARNING: ASAN interferes")) | |
| .join("\n") | |
| ).toBe(""); | |
| expect(stdout.trim()).toBe("ok"); | |
| expect(exitCode).toBe(0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/resolve/require-esm-gc-roots.test.ts` around lines 58 - 61, The
test currently asserts stderr is empty but ASAN startup warnings from the
spawned subprocess can cause flakes; before asserting on stderr (the variable
produced from proc.stderr.text()), filter out the known ASAN startup noise (e.g.
remove lines matching /WARNING: ASAN.*JSC signal handlers.*\n?/ or a broader
/WARNING: ASAN.*/ pattern) and trim the result, then assert the cleaned stderr
is empty; update the block that reads proc.stdout.text(), proc.stderr.text(),
proc.exited and replace the direct expect(stderr).toBe("") with an assertion on
the filtered/trimmed stderr string.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(JSON.parse(stdout.trim())).toEqual(["after-require", "then-before-require", "await-inside-esm"]); |
There was a problem hiding this comment.
Filter ASAN startup noise before asserting on stderr.
This subprocess executes JS, so debug/ASAN lanes can emit the one-line WARNING: ASAN interferes... banner even with bunEnv. The raw empty-stderr assertion will fail there.
Suggested fix
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
- expect(stderr).toBe("");
+ const filteredStderr = stderr
+ .split("\n")
+ .filter(line => !line.startsWith("WARNING: ASAN interferes"))
+ .join("\n")
+ .trim();
+ expect(filteredStderr).toBe("");
expect(JSON.parse(stdout.trim())).toEqual(["after-require", "then-before-require", "await-inside-esm"]);
expect(exitCode).toBe(0);Based on learnings: In oven-sh/bun test files that spawn subprocesses using bunEnv, suppress the known ASAN startup noise in the subprocess stderr before asserting it is empty, using the repo’s filter-based convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/resolve/require-esm-microtask-order.test.ts` around lines 40 -
42, The test currently asserts stderr is exactly empty but can fail due to ASAN
startup banner emitted by the subprocess; before the expect on `stderr` (after
awaiting `proc.stdout.text()`/`proc.stderr.text()`/`proc.exited`), pass `stderr`
through the repo’s subprocess-stderr filter (the shared ASAN-startup filter
helper used across tests, e.g. filterSubprocessStderr or equivalent) to strip
the known `WARNING: ASAN interferes...` line, then assert the filtered result is
empty; reference `proc`, `stdout`, and `stderr` from this test and use the
existing filter helper rather than changing environment setup.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(stdout.trim()).toBe("ok"); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
Filter ASAN startup noise before asserting on stderr.
Both subprocesses execute JS, so the raw empty-stderr assertions are brittle on ASAN/debug lanes for the same reason as other test/js/bun/** subprocess tests.
Suggested fix
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
- expect(stderr).toBe("");
+ const filteredStderr = stderr
+ .split("\n")
+ .filter(line => !line.startsWith("WARNING: ASAN interferes"))
+ .join("\n")
+ .trim();
+ expect(filteredStderr).toBe("");
expect(stdout.trim()).toBe("ok");
expect(exitCode).toBe(0);Based on learnings: In oven-sh/bun test files that spawn subprocesses using bunEnv, suppress the known ASAN startup noise in the subprocess stderr before asserting it is empty, using the repo’s filter-based convention.
Also applies to: 84-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/resolve/require-esm-transitive-tla.test.ts` around lines 44 - 47,
The test currently asserts raw stderr is empty which is brittle on ASAN/debug
lanes; after awaiting proc.stderr.text() (the variable stderr from the
Promise.all that also produces stdout and exitCode), pass stderr through the
repo's subprocess-stderr filter function used in other bun tests (e.g., the
conventional filter that strips ASAN startup noise) and assert the filtered
result is empty instead of the raw stderr; apply the same change for the other
occurrence around lines 84-87 where stderr is asserted empty.
| // Kick off the async load first so the registry entry exists. | ||
| const p = import("./side.mjs"); | ||
| // Yield to a macro-task so the loader has fetched + entered evaluation | ||
| // (status EvaluatingAsync) but the TLA setTimeout(20) is still pending. | ||
| await new Promise(r => setTimeout(r, 1)); | ||
| // The new loader throws "async module"; the old JS loader returned a | ||
| // partial namespace. Either way the registry entry must survive. | ||
| try { require("./side.mjs"); } catch {} |
There was a problem hiding this comment.
Replace the 1ms timing guess with a deterministic readiness check.
This can pass vacuously on slower builders: if the outer import() has not reached EvaluatingAsync yet, require() no longer exercises the ownership/deletion path this test is meant to cover.
Suggested fix
// Kick off the async load first so the registry entry exists.
const p = import("./side.mjs");
- // Yield to a macro-task so the loader has fetched + entered evaluation
- // (status EvaluatingAsync) but the TLA setTimeout(20) is still pending.
- await new Promise(r => setTimeout(r, 1));
+ // Wait until side.mjs has actually started evaluating and is suspended
+ // on its TLA, instead of relying on a timing guess.
+ while (globalThis.__sideEvalCount !== 1) {
+ await Promise.resolve();
+ }
// The new loader throws "async module"; the old JS loader returned a
// partial namespace. Either way the registry entry must survive.
try { require("./side.mjs"); } catch {}As per coding guidelines: "Do not write flaky tests; do not use setTimeout in tests - instead await the condition to be met".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/resolve/require-esm-transitive-tla.test.ts` around lines 60 - 67,
Replace the flaky setTimeout wait with a deterministic readiness check: have the
async module (side.mjs) expose a readiness signal (e.g., export a Promise or
boolean that is resolved/set when it reaches the EvaluatingAsync/TLA-started
point) and in the test await that signal after starting the top-level import
(the import("./side.mjs") call) before calling require("./side.mjs"); this
ensures the test only proceeds once the module is in the expected
EvaluatingAsync state rather than relying on a timing guess.
| // JSC's C++ module loader keeps a handful of pipeline JSPromises live in the | ||
| // module map (fetch/module/load per registry entry) for the life of the | ||
| // process. These are constant across iterations, so account for them | ||
| // separately from the per-batch leak threshold. | ||
| const promiseThreshold = threshold + 10; |
There was a problem hiding this comment.
Avoid hardcoding the loader-promise allowance.
+ 10 bakes today's module-loader internals into a leak test. The next WebKit roll can turn this into either a flaky failure or a false negative that tolerates real Promise growth. Capture the initial Promise count once and assert against baseline + threshold instead.
♻️ Suggested shape
const threshold = batch * 2 + batch / 2;
-const promiseThreshold = threshold + 10;
+const initialPromiseCount = getHeapStats().Promise || 0;
+const promiseThreshold = initialPromiseCount + threshold;Also applies to: 113-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/web/fetch/fetch-leak-test-fixture-5.js` around lines 11 - 15,
Currently promiseThreshold is set by hardcoding "+ 10", which bakes loader
internals into the test; instead capture the initial Promise count once and use
that as the baseline. Replace the hardcoded const promiseThreshold = threshold +
10; by recording the initial Promise count (e.g., capture a baselinePromises
count before test iterations) and compute the allowed limit as baselinePromises
+ threshold wherever promiseThreshold is used; update references to use this
computed baseline + threshold so the test adapts to current loader behavior
rather than assuming "+ 10".
|
|
||
| /// Module loader registry entry (new C++ module loader). | ||
| ModuleRegistryEntry = 29, | ||
|
|
||
| /// Module loading context (new C++ module loader). | ||
| ModuleLoadingContext = 30, | ||
|
|
||
| /// Module loader payload (new C++ module loader). | ||
| ModuleLoaderPayload = 31, | ||
|
|
||
| /// Module graph loading state (new C++ module loader). | ||
| ModuleGraphLoadingState = 32, | ||
|
|
||
| /// JSModuleLoader cell type (new C++ module loader). | ||
| JSModuleLoader = 33, | ||
|
|
||
| /// Base JavaScript object type. | ||
| /// ```js | ||
| /// {} |
There was a problem hiding this comment.
🔴 Five new internal JSC cell types introduced by this PR (ModuleRegistryEntry=29, ModuleLoadingContext=30, ModuleLoaderPayload=31, ModuleGraphLoadingState=32, JSModuleLoader=33) are missing from the isHidden() predicate in src/bun.js/bindings/JSType.zig. Without this guard, if any of these cells reaches console.log or the test-runner's pretty-printer, the formatter falls through to else => .JSON and attempts to JSON-serialize a non-JS-object internal cell, which can crash or produce garbage output.
Extended reasoning...
What the bug is
The PR adds five new JSType enum values for the new C++ module loader: ModuleRegistryEntry=29, ModuleLoadingContext=30, ModuleLoaderPayload=31, ModuleGraphLoadingState=32, and JSModuleLoader=33. These were inserted before Object=34 (shifting all existing object-type ordinals by +5). The PR correctly renumbers all downstream variants in JSType.zig, but neglects to add these five new types to the isHidden() predicate.
The specific code path
isHidden() is the primary guard in ConsoleObject.zig (lines 1232, 1947, 3175) and test/pretty_format.zig (lines 369, 775, 1566). When it returns false for a value, the formatters proceed to determine what the value is. Since these new cell types have ordinals 29–33, isObject() returns false for all of them (Object = 34 is the boundary). They are not in canGet(). They don't match any explicit arm in the large switch statement at ConsoleObject.zig:1320. The result: the formatter falls to else => .JSON (ConsoleObject.zig:1410) and calls JSC's JSON serializer on an internal JSCell that is not a JS object.
Why existing code doesn't prevent it
The analogous predecessor types that these cells replace — JSSourceCode=22, JSScriptFetcher=23, JSScriptFetchParameters=24 — are all explicitly listed in isHidden(). The same applies to PromiseReaction=25, PromiseAllContext=26, PromiseAllGlobalContext=27. The PR introduces the five new types but doesn't extend the guard. There is even a comment in ConsoleObject.zig (around line 1377): 'None of these should ever exist here / But we are going to check anyway' — written precisely because the existing types were added to isHidden() defensively.
Impact
If any of the new loader cells becomes observable as a JSValue — e.g., through the debugger/inspector (getModuleGraph), an accidentally-exposed internal value, or a future code path — the formatter tries to serialize it as JSON. Calling JSValue::synthesizePrototype or the JSON stringifier on a bare JSCell (not a JSObject) is undefined behavior and can crash the process, corrupt memory, or emit garbage. The PR itself sets exposeInternalModuleLoader() = false to reduce direct exposure, but the defensive isHidden() guard is still the correct safety net.
Step-by-step proof
- Any of the five new cell types reaches
ConsoleObject::formatJSValue. isHidden()is checked first: the new types are absent from its arm list, so it returnsfalse.isObject()check: ordinals 29–33 < 34 (Object), so returnsfalse.canGet()check: none of the new types appear there, returnsfalse.- The explicit NativeCode arm (lines 1377–1401) lists
JSSourceCode,JSScriptFetcher,JSScriptFetchParameters— but not the new types. - Falls to
else => .JSONat line 1410 — JSON serialization of an internal JSCell.
Fix
Add ModuleRegistryEntry, ModuleLoadingContext, ModuleLoaderPayload, ModuleGraphLoadingState, and JSModuleLoader to the arm list inside isHidden() in src/bun.js/bindings/JSType.zig.
🔬 also observed by coderabbitai
| auto* entry = global->moduleLoader()->registryEntry(keyIdent); | ||
| ASSERT(entry); // should have called BakeLoadServerCode and wait for that promise | ||
| auto* module = entry ? entry->record() : nullptr; | ||
| ASSERT(module); | ||
| JSC::JSModuleNamespaceObject* namespaceObject = global->moduleLoader()->getModuleNamespaceObject(global, module); | ||
| ASSERT(namespaceObject); | ||
| return JSC::JSValue::encode(namespaceObject); |
There was a problem hiding this comment.
🔴 In BakeGetModuleNamespace (BakeSourceProvider.cpp:118–124), registryEntry() can return null, and the code explicitly converts that to a null AbstractModuleRecord* via ternary — then passes it directly to getModuleNamespaceObject(), which unconditionally dereferences it. The only guards are ASSERT(entry) and ASSERT(module), both of which are no-ops in release builds, meaning this causes a hard crash in production when a module key is missing or loading failed. The secondary caller BakeGetDefaultExportFromModule (line 132) performs an immediate jsCast on the (potentially null-encoding) encoded return value, creating a second crash path with no null check at all.
Extended reasoning...
What the bug is and how it manifests
BakeGetModuleNamespace (BakeSourceProvider.cpp:111-125) retrieves a module namespace object from the JSC module registry. It calls global->moduleLoader()->registryEntry(keyIdent) to look up the registry entry for the given module key. The registryEntry() accessor can legitimately return nullptr — for example, if the module has not been registered yet, if the promise returned by BakeLoadModuleByKey was not awaited before calling this function, or if the module's evaluation failed and its registry entry was removed. When that happens, the ternary on line 120 explicitly produces a null AbstractModuleRecord*: auto* module = entry ? entry->record() : nullptr;.
The specific code path that triggers it
The null pointer flows directly into global->moduleLoader()->getModuleNamespaceObject(global, module) on line 122. Looking at the JSC implementation, getModuleNamespaceObject calls module->getModuleNamespace(global) unconditionally — it does not perform a null check on the AbstractModuleRecord* parameter. This dereferences the null pointer, causing a segmentation fault / access violation. A second crash path exists in BakeGetDefaultExportFromModule (line 127-133), which calls BakeGetModuleNamespace and immediately jsCasts the encoded return value to JSModuleNamespaceObject* with no null or validity check between the two calls.
Why existing code doesn't prevent it
The only safeguards in place are ASSERT(entry) at line 119 and ASSERT(module) at line 121, together with a code comment // should have called BakeLoadServerCode and wait for that promise. In debug builds these assertions fire and produce a useful crash message. However, ASSERT in WebKit/JSC is compiled out entirely in release builds — it expands to nothing. The ternary expression on line 120 is itself evidence that the author acknowledged the null case (otherwise a simple entry->record() dereference would suffice), but the code then proceeds to pass that acknowledged null value into a function that dereferences it. Compare line 79 and 107 in the same file, where RELEASE_ASSERT(result) is used — those survive into release builds. The missing RELEASE_ASSERT pattern here is the omission.
What the impact would be
Any caller that reaches BakeGetModuleNamespace (or BakeGetDefaultExportFromModule) while the target module is not in the registry will crash the Bun server process with a null dereference in release builds. In Bake's production server mode, this is likely to occur during error recovery scenarios: if a module fails to evaluate, the registry entry may be absent; similarly, race conditions or ordering bugs in the callers could trigger this. Because the ASSERTs provide no protection in release, the developer-visible debug crash is silent in the shipped binary.
How to fix it
Replace the ASSERT-only guards with either (a) RELEASE_ASSERT(entry) / RELEASE_ASSERT(module) to crash loudly with a meaningful message in release builds too, or (b) a proper null check that throws a JavaScript exception via the scope machinery and returns an encoded exception value, which would allow Bun to surface the error to user code rather than killing the process. For BakeGetDefaultExportFromModule, the intermediate encoded value should be checked (e.g., via JSValue::decode and an isNull() / isEmpty() guard) before the jsCast.
Step-by-step proof
- Suppose a Bake production server encounters a module at path
bake://routes/missing.jsthat was never registered (e.g., due to a bundler error or skipped chunk). - Zig production code calls
BakeGetModuleNamespace(global, key)wherekeyencodes"bake://routes/missing.js". registryEntry(keyIdent)scans the module registry and finds no entry → returnsnullptr.ASSERT(entry)→ no-op in release → execution continues.auto* module = entry ? entry->record() : nullptr;→moduleisnullptr.ASSERT(module)→ no-op in release → execution continues.global->moduleLoader()->getModuleNamespaceObject(global, nullptr)is called → the implementation callsmodule->getModuleNamespace(global)→ null pointer dereference → SIGSEGV / process crash.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: This change introduced a standalone constant WEBKIT_VERSION instead of
following the repo's dependency bump convention; update the module to export a
commit field named commit with the new value (replace or remove WEBKIT_VERSION
and add export const commit = "autobuild-preview-pr-190-f8513c72") so other
build tooling that reads the commit field in scripts/build/deps/<name>.ts picks
up the bump; ensure no other references to WEBKIT_VERSION remain or are updated
to use commit.
- Line 6: The current WEBKIT_VERSION value is a non-unique preview tag which,
when truncated by prebuiltDestDir(), causes many different preview builds to map
to the same webkit-${slice(0,16)} directory and leads to cache collisions; fix
by making the prebuilt destination stable and unique for preview-style
tags—either change WEBKIT_VERSION to include a stable full-commit SHA or update
prebuiltDestDir() to derive its directory from the full version string (or a
deterministic hash of it) instead of slice(0,16), so each distinct preview tag
maps to a distinct prebuilt directory.
🪄 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: 5bfcda5c-862b-4837-b933-25d3a5ab55d4
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "4d5e75ebd84a14edbc7ae264245dcd77fe597c10"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-190-f8513c72"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align dependency bump flow with repo convention (commit field).
This bump updates a standalone constant rather than a dependency commit field, which diverges from the build-script dependency bump convention.
As per coding guidelines, "Bump dependencies by editing the commit field in scripts/build/deps/<name>.ts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build/deps/webkit.ts` at line 6, This change introduced a standalone
constant WEBKIT_VERSION instead of following the repo's dependency bump
convention; update the module to export a commit field named commit with the new
value (replace or remove WEBKIT_VERSION and add export const commit =
"autobuild-preview-pr-190-f8513c72") so other build tooling that reads the
commit field in scripts/build/deps/<name>.ts picks up the bump; ensure no other
references to WEBKIT_VERSION remain or are updated to use commit.
Prevent prebuilt cache collisions for preview-style WebKit versions.
With Line 6 using a non-SHA tag, prebuiltDestDir() (Line 85) now collapses many versions to the same webkit-${slice(0,16)} directory (autobuild-previe...). That can cause cache stomping/churn and nondeterministic behavior when different preview tags are built concurrently.
💡 Proposed fix
function prebuiltDestDir(cfg: Config): string {
- const version16 = cfg.webkitVersion.slice(0, 16);
- return resolve(cfg.cacheDir, `webkit-${version16}${prebuiltSuffix(cfg)}`);
+ // Use full identity stability for non-SHA tags (e.g. autobuild-preview-pr-*)
+ // to avoid cache key collisions across distinct WebKit versions.
+ return resolve(cfg.cacheDir, `webkit-${cfg.webkitVersion}${prebuiltSuffix(cfg)}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build/deps/webkit.ts` at line 6, The current WEBKIT_VERSION value is
a non-unique preview tag which, when truncated by prebuiltDestDir(), causes many
different preview builds to map to the same webkit-${slice(0,16)} directory and
leads to cache collisions; fix by making the prebuilt destination stable and
unique for preview-style tags—either change WEBKIT_VERSION to include a stable
full-commit SHA or update prebuiltDestDir() to derive its directory from the
full version string (or a deterministic hash of it) instead of slice(0,16), so
each distinct preview tag maps to a distinct prebuilt directory.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/build/deps/webkit.ts (2)
6-6: 🛠️ Refactor suggestion | 🟠 MajorUse a
commitexport for dependency bumps in this file.At Line 6, the bump is done via
WEBKIT_VERSION. Inscripts/build/deps/*.ts, bumps should be done through acommitfield export so tooling and conventions stay consistent.💡 Proposed fix
-export const WEBKIT_VERSION = "autobuild-preview-pr-190-1987e88f"; +export const commit = "autobuild-preview-pr-190-1987e88f"; +export const WEBKIT_VERSION = commit;As per coding guidelines, "Bump dependencies by editing the
commitfield inscripts/build/deps/<name>.ts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build/deps/webkit.ts` at line 6, Replace the current WEBKIT_VERSION export with a standardized commit export: remove or stop using the WEBKIT_VERSION constant and instead export a const named commit with the same string value (i.e., set commit = "autobuild-preview-pr-190-1987e88f"); update any local references in this file to use the commit symbol if needed so the file follows the scripts/build/deps/* convention.
6-6:⚠️ Potential issue | 🟠 MajorAvoid cache-key collisions for preview WebKit versions.
At Line 6, this preview-style value shares a long common prefix with other preview tags, but
prebuiltDestDir()truncates to 16 chars (Line 85). Different preview builds can map to the samewebkit-*cache directory and stomp each other.💡 Proposed fix
function prebuiltDestDir(cfg: Config): string { - const version16 = cfg.webkitVersion.slice(0, 16); - return resolve(cfg.cacheDir, `webkit-${version16}${prebuiltSuffix(cfg)}`); + const isShaLike = /^[0-9a-f]{40}$/i.test(cfg.webkitVersion); + const cacheKey = isShaLike ? cfg.webkitVersion.slice(0, 16) : cfg.webkitVersion; + return resolve(cfg.cacheDir, `webkit-${cacheKey}${prebuiltSuffix(cfg)}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build/deps/webkit.ts` at line 6, The preview WEBKIT_VERSION value causes cache-key collisions because prebuiltDestDir() truncates the version to 16 characters, so different preview tags with a long common prefix map to the same webkit-* directory; update the scheme so the cache directory remains unique by either changing WEBKIT_VERSION to include a short unique suffix/hash (e.g., append the PR number or a short commit hash) or by modifying prebuiltDestDir() to use a longer/complete identifier for building the webkit-* directory name; locate the symbol WEBKIT_VERSION in scripts/build/deps/webkit.ts and the function prebuiltDestDir() and ensure the chosen fix preserves deterministic naming and avoids truncation collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: Replace the current WEBKIT_VERSION export with a standardized commit
export: remove or stop using the WEBKIT_VERSION constant and instead export a
const named commit with the same string value (i.e., set commit =
"autobuild-preview-pr-190-1987e88f"); update any local references in this file
to use the commit symbol if needed so the file follows the scripts/build/deps/*
convention.
- Line 6: The preview WEBKIT_VERSION value causes cache-key collisions because
prebuiltDestDir() truncates the version to 16 characters, so different preview
tags with a long common prefix map to the same webkit-* directory; update the
scheme so the cache directory remains unique by either changing WEBKIT_VERSION
to include a short unique suffix/hash (e.g., append the PR number or a short
commit hash) or by modifying prebuiltDestDir() to use a longer/complete
identifier for building the webkit-* directory name; locate the symbol
WEBKIT_VERSION in scripts/build/deps/webkit.ts and the function
prebuiltDestDir() and ensure the chosen fix preserves deterministic naming and
avoids truncation collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5e4cdf4-face-45b0-8f99-6c1513685f7c
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/build/deps/webkit.ts (1)
6-6:⚠️ Potential issue | 🟠 MajorAvoid cache-key collisions when using preview WebKit tags.
Line 6 now uses a preview-style version string, but
prebuiltDestDir()still truncatescfg.webkitVersionto 16 chars. Forautobuild-preview-pr-*, this collapses multiple versions to the samewebkit-autobuild-previe...directory and can reuse the wrong prebuilt artifacts.💡 Proposed fix
function prebuiltDestDir(cfg: Config): string { - const version16 = cfg.webkitVersion.slice(0, 16); - return resolve(cfg.cacheDir, `webkit-${version16}${prebuiltSuffix(cfg)}`); + const isCommitSHA = /^[0-9a-f]{40}$/i.test(cfg.webkitVersion); + const versionKey = isCommitSHA ? cfg.webkitVersion.slice(0, 16) : cfg.webkitVersion; + return resolve(cfg.cacheDir, `webkit-${versionKey}${prebuiltSuffix(cfg)}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build/deps/webkit.ts` at line 6, prebuiltDestDir() currently truncates cfg.webkitVersion to 16 chars which causes different preview-style tags like WEBKIT_VERSION = "autobuild-preview-pr-195-1d58eec1" to collapse into the same directory; update prebuiltDestDir() so it preserves the full cfg.webkitVersion (or at least does not truncate when the version startsWith "autobuild-preview") to avoid cache-key collisions—locate the truncation logic referencing cfg.webkitVersion in prebuiltDestDir and remove or conditionally bypass the .slice(0,16) behavior so each preview tag maps to a unique dest dir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: prebuiltDestDir() currently truncates cfg.webkitVersion to 16 chars
which causes different preview-style tags like WEBKIT_VERSION =
"autobuild-preview-pr-195-1d58eec1" to collapse into the same directory; update
prebuiltDestDir() so it preserves the full cfg.webkitVersion (or at least does
not truncate when the version startsWith "autobuild-preview") to avoid cache-key
collisions—locate the truncation logic referencing cfg.webkitVersion in
prebuiltDestDir and remove or conditionally bypass the .slice(0,16) behavior so
each preview tag maps to a unique dest dir.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2ebb407-fac2-4fb6-9b6a-baabe548cc37
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
|
|
||
| auto* promise = JSC::loadAndEvaluateModule(globalObject, name, JSC::jsUndefined(), JSC::jsUndefined()); | ||
| EXCEPTION_ASSERT(!!promise == !scope.exception()); | ||
| if (!promise) return nullptr; | ||
|
|
||
| JSC::JSNativeStdFunction* resolverFunction = JSC::JSNativeStdFunction::create( | ||
| vm, globalObject, 1, String(), resolverFunctionCallback); | ||
|
|
||
| auto* newPromise = promise->then(globalObject, resolverFunction, globalObject->promiseEmptyOnRejectedFunction()); | ||
| EXCEPTION_ASSERT(!!scope.exception() == !newPromise); | ||
| return newPromise; | ||
| return promise; |
There was a problem hiding this comment.
🟡 This PR removed the only call sites of the two static resolverFunctionCallback helpers (the JSNativeStdFunction::create(..., resolverFunctionCallback) + promise->then(...) chains in JSC__JSModuleLoader__loadAndEvaluateModule and jsFunctionRunMain), but left the function definitions behind at src/bun.js/bindings/bindings.cpp:3476 and src/bun.js/modules/NodeModuleModule.cpp:787. Both are now unreferenced dead code — Bun's build sets -Wno-unused-function so this doesn't break CI, but the orphaned definitions should be deleted along with their call sites.
Extended reasoning...
What the bug is
This PR simplified two functions that previously chained a no-op .then() onto the module-load promise:
JSC__JSModuleLoader__loadAndEvaluateModuleinsrc/bun.js/bindings/bindings.cpp— now returnspromisedirectly at line 3492 instead of creating aJSNativeStdFunctionfromresolverFunctionCallbackand callingpromise->then(resolverFunction, ...).jsFunctionRunMaininsrc/bun.js/modules/NodeModuleModule.cpp— now passespromisedirectly toBun__VirtualMachine__setOverrideModuleRunMainPromiseat line 802 instead of routing through the same.then(resolverFunction, ...)pattern.
In both cases the PR deleted the call site but left the static helper definition just above it intact. Grep confirms each file now contains exactly one occurrence of resolverFunctionCallback — the definition itself.
Why this doesn't break the build
Bun's compiler flags explicitly suppress this warning: scripts/build/flags.ts:471 passes -Wno-unused-function (alongside -Werror=unused in debug builds), and src/bun.js/bindings/helpers.h:23 (included by bindings.cpp) carries #pragma clang diagnostic ignored "-Wunused-function". So neither file emits a warning or error, and CI is building cleanly per the robobun comment on commit 70e37dc.
Impact
Zero functional or build impact. This is purely dead-code cleanup — two ~4-line static functions that compile to nothing and are never referenced.
Step-by-step proof
- Before this PR,
bindings.cppcontained:The diff replaces this withJSC::JSNativeStdFunction* resolverFunction = JSC::JSNativeStdFunction::create( vm, globalObject, 1, String(), resolverFunctionCallback); auto* newPromise = promise->then(globalObject, resolverFunction, ...); return newPromise;
return promise;. The staticresolverFunctionCallbackat line 3476 was thatcreate()call's only reference. - Before this PR,
NodeModuleModule.cpphad the same pattern injsFunctionRunMain; the diff replacespromise->then(resolverFunction, ...)with a directBun__VirtualMachine__setOverrideModuleRunMainPromise(..., promise). The staticresolverFunctionCallbackat line 787 was that call's only reference. rg resolverFunctionCallback src/now returns only the two definition lines (bindings.cpp:3476, NodeModuleModule.cpp:787) — no callers remain in either file or anywhere else.- Both functions are file-scope
static, so no other translation unit can reference them.
Fix
Delete both static function definitions:
src/bun.js/bindings/bindings.cpp:3476-3480src/bun.js/modules/NodeModuleModule.cpp:787-790
Jarred-Sumner
force-pushed
the
claude/webkit-upgrade-87fd0daba19a
branch
from
70e37dc to
8af1aec
Compare
| /* Legacy implementation removed: relied on the old JS-side JSModuleLoader | ||
| * (Loader.registry JSMap, $setStateToMax, parseModule, etc.) which no longer | ||
| * exists after the upstream module-loader rewrite. | ||
| function loadEsmIntoCjs__dead(resolvedSpecifier: string) { | ||
| var loader = Loader; |
There was a problem hiding this comment.
🟡 The new loadEsmIntoCjs() is now a one-liner calling $esmLoadSync, but the entire ~130-line legacy implementation is left behind as a /* ... */ block comment (loadEsmIntoCjs__dead, lines 180–311). It references removed APIs (Loader.registry, $setStateToMax, parseModule, requestFetch) that no longer exist after the module-loader rewrite, so it can never be revived in-place — git history is the right home for it, and the block should just be deleted.
Extended reasoning...
What the issue is
The PR replaces loadEsmIntoCjs() with a one-line call to the new $esmLoadSync intrinsic (CommonJS.ts:170–178). However, instead of deleting the old ~130-line implementation, it wraps the entire body in a /* ... */ block comment, renames it to loadEsmIntoCjs__dead, and prefixes it with a header that says "Legacy implementation removed" — while leaving all 130 lines in place (lines 180–311).
The specific code
/* Legacy implementation removed: relied on the old JS-side JSModuleLoader
* (Loader.registry JSMap, $setStateToMax, parseModule, etc.) which no longer
* exists after the upstream module-loader rewrite.
function loadEsmIntoCjs__dead(resolvedSpecifier: string) {
var loader = Loader;
var queue = $createFIFO();
...
return registry.(resolvedSpecifier);
}
*/The commented-out body references Loader.registry, $setStateToMax, loader.parseModule, loader.requestedModules, loader.ensureRegistered, loader.linkAndEvaluateModule, entry.dependenciesMap, etc. — all of which were removed by the upstream module-loader rewrite (JSModuleLoader is now a JSCell with no JS-visible registry property; the ModuleLoader.js builtins are gone). Uncommenting this block would not even compile against the new builtins, so it cannot be "revived" in-place.
Why nothing catches this
Because it's inside a block comment, the builtins codegen strips it before compilation and there is zero runtime impact. No lint rule in this repo flags large commented-out code blocks.
Impact
Purely source-hygiene: ~130 lines of dead bloat in a hand-maintained builtin file. It mildly confuses readers (the header says "removed" but the code is still there) and adds noise to future diffs/greps for symbols like $setStateToMax or Loader.registry that this PR is otherwise eliminating from the codebase.
How to fix
Delete lines 180–311 of src/js/builtins/CommonJS.ts (the entire /* Legacy implementation removed ... */ block). Git history already preserves the old implementation; that's the canonical place to recover it from if ever needed.
Step-by-step proof
src/js/builtins/CommonJS.ts:170–178— the new exportedloadEsmIntoCjsis complete:return $esmLoadSync(resolvedSpecifier);.- Line 180 opens a
/*block comment with the header "Legacy implementation removed". - Line 183 begins
function loadEsmIntoCjs__dead(resolvedSpecifier: string) {— note the__deadsuffix and absence ofexport, confirming the author already considers it dead. - Lines 184–309 contain the full old body, referencing
Loader.registry,$setStateToMax(entry, $ModuleLink),loader.parseModule,loader.ensureRegistered,loader.linkAndEvaluateModule— none of which exist after this PR (the diff removesLoaderfromBunBuiltinNames.h/addBuiltinGlobalsand setsexposeInternalModuleLoader() = false). - Line 311 closes the comment with
*/. - Result: ~130 lines of source that compile to nothing and reference an API surface the same PR deletes. Git history is the appropriate archive.
| macro(esmNamespaceForCjs) \ | ||
| macro(esmRegistryDelete) \ | ||
| macro(esmRegistryEvaluatedKeys) \ | ||
| macro(esmLoadSync) \ |
There was a problem hiding this comment.
🟡 The four new esm* identifiers are still appended after fulfillModuleSync (lines 105-108), violating the "Keep this list sorted" rule at line 23 — they belong between errorSteps and evaluateCommonJSModule, and esmLoadSync should precede esmNamespaceForCjs among themselves. Re-raising because the earlier CodeRabbit thread on this was auto-marked "✅ Addressed in 912405a..8af1aec" and resolved, but the current diff shows it was not actually fixed.
Extended reasoning...
What the issue is
BunBuiltinNames.h line 23 carries the explicit instruction // Keep this list sorted. for the BUN_COMMON_PRIVATE_IDENTIFIERS_EACH_PROPERTY_NAME macro list. The four new entries added by this PR — esmNamespaceForCjs, esmRegistryDelete, esmRegistryEvaluatedKeys, esmLoadSync — sit at lines 105-108, immediately after fulfillModuleSync (line 104) and before getInternalWritableStream (line 109). Since "esm" < "eva" < "f", they belong between errorSteps (line 92) and evaluateCommonJSModule (line 93). They are also unsorted among themselves: alphabetically the order should be esmLoadSync, esmNamespaceForCjs, esmRegistryDelete, esmRegistryEvaluatedKeys.
Why this is being re-raised despite an existing comment
CodeRabbit inline-comment id=3098005682 already flagged this, and one verifier refuted this report on duplicate grounds. However, that thread is currently marked resolved with the auto-generated note "✅ Addressed in commits 912405a to 8af1aec". That marker is factually wrong — the file at HEAD (commit 8af1aec) still has the entries at lines 105-108, after fulfillModuleSync. CodeRabbit's auto-detection appears to have matched on an unrelated change in that commit range. Because the thread is resolved and labeled addressed, the author has every reason to believe it's done; a fresh comment is the only way to surface that it isn't. This is materially different from duplicating an open comment.
Why existing code doesn't prevent it
The macro is purely a list of token arguments expanded by macro(...); the C++ preprocessor and compiler impose no ordering constraint, so nothing in the build catches a sort-order violation. The convention exists only to keep merge conflicts and manual lookup tractable.
Impact
None functionally — this is a documentation/convention violation only. The generated private-identifier table works identically regardless of order. Hence nit severity.
How to fix
Move the four lines from 105-108 to between line 92 (errorSteps) and line 93 (evaluateCommonJSModule), reordered as:
macro(errorSteps) \
macro(esmLoadSync) \
macro(esmNamespaceForCjs) \
macro(esmRegistryDelete) \
macro(esmRegistryEvaluatedKeys) \
macro(evaluateCommonJSModule) \Step-by-step proof
- Line 23 of
src/js/builtins/BunBuiltinNames.hreads// Keep this list sorted. - Lines 92-96 read:
errorSteps,evaluateCommonJSModule,evictIsolationSourceProviderCache,expires,exports— i.e., the existinge*block, with noesm*entries. - Lines 102-109 read:
flushAlgorithm,format,fulfillModuleSync,esmNamespaceForCjs,esmRegistryDelete,esmRegistryEvaluatedKeys,esmLoadSync,getInternalWritableStream. - Lexicographic comparison:
"esm..."<"evaluateCommonJSModule"<"fulfillModuleSync", so placement afterfulfillModuleSyncviolates the sort. Within the four,"esmLoadSync" < "esmNamespaceForCjs"butesmLoadSyncappears last. - The CodeRabbit thread on this exact issue carries
resolved="true"and "✅ Addressed in commits 912405a to 8af1aec", yet HEAD is 8af1aec and steps 2-3 above are read directly from it.
…ade oven-sh#29393) Two follow-on fixes the upstream merge made necessary; clean to keep separate from the merge commit (fb079df) so the merge stays a pure integration point. scripts/build/unified.ts + 8 parabun_*.cpp files added to the noUnify list. Para's host-function bindings rely on `using namespace JSC;` plus file-static helpers (`xioctl`, etc.) that collide when bundled into a unified TU alongside upstream's other .cpp files. scripts/build/zig.ts - Upstream removed `usingParallelCompiler()` (collapsed the dual-compiler split) and renamed `codegenThreadCount` → `codegenThreads`. Combined Para's memory-aware capping logic (PARABUN_LLVM_CODEGEN_THREADS env override, 1.5 GB/thread cap) with upstream's windows/lto/CI-asan gates. Net: same call sites on both sides resolve to one function with both behaviors. src/bun.js/bindings/parabun_*.cpp (80 sites across 8 files) WebKit upgrade (oven-sh#29393) removed `JSC::jsCast` and `JSC::jsDynamicCast`. Mechanical replace with `dynamicDowncast<T>(value)` (drop `*` from template arg — function adds it). All Para call sites already pre-validate the type, so the new null-returning semantics don't introduce real error paths. Verified: 64 pass / 2 skip / 0 fail across 10 test files (signals/resource, audio/signals, camera/signals, vision/motion-signals, speech/listen-signals, llm/signals, gpio/api, i2c/api, spi/api, assistant/assistant). Skips are hardware/fixture gated, same posture as before merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`loadEntryPoint` waited on the entry's evaluation promise via `waitForPromise`, which loops `tick(); autoTick();` until the promise settles. With the spec-aligned JSC module loader (#29393), a TLA self-cycle (or any `await` on a never-settling promise) leaves that promise pending forever, so `waitForPromise` busy-spun on `tickWithoutIdle` and the process hung. The never-settling-promise case predates #29393; the cycle case used to resolve under the old loader, which masked it. Fix: in `loadEntryPoint`, break the wait once the event loop is idle so control returns to the main run loop. There, after `onBeforeExit` fully drains (and a final microtask drain so a beforeExit handler can still settle the await — Node does), check whether the entry promise is still pending; if so, warn and exit 13 to match Node's ERR_UNSETTLED_TOP_LEVEL_AWAIT behaviour. JSC has no equivalent of V8's `GetStalledTopLevelAwaitMessages`, so the warning names the entry path rather than the exact await location.
Fixes #30281. Bumps WebKit to oven-sh/WebKit#217 (preview build `autobuild-preview-pr-217-ae5228e0`). ## What `require()` of an ESM file that statically imports `react` + an MUI v9 forwardRef sub-module (`Typography`, `DialogContent`, etc) aborted on the `m_status == Status::Fetching` assertion at `ModuleRegistryEntry.cpp:254` — SIGABRT on Linux, arm64 PAC IB trap (SIGTRAP) on macOS. Running the same file as the ESM entry (`bun repro.js`) was fine; the bug was on the CommonJS-`require()`-of-ESM path added by #29393. ## Why `moduleRegistryModuleSettled` fired twice for the same `ModuleRegistryEntry`: (1) inline during `hostLoadImportedModule`'s synchronous-replay branch (`fetchPromise` is Fulfilled) where `makeModule` + `fetchComplete` + `modulePromise->fulfillPromise` run synchronously while a `require(esm)` drains the synchronous module queue; (2) from the normal microtask queue for the stale `ModuleRegistryModuleSettled` reaction that `moduleRegistryFetchSettled` had queued *before* the sync mode was entered. `moduleRegistryFetchSettled` already guarded this exact shape with `if (modulePromise->status() != Pending) return`; `moduleRegistryModuleSettled` did not. ## How WebKit-side (oven-sh/WebKit#217): apply the same `modulePromise` pending-guard symmetrically to `moduleRegistryModuleSettled`. Bun-side: `WEBKIT_VERSION` bump + regression test. ## Tests `test/regression/issue/30281.test.ts` installs `react@19` + `@mui/material@9` + the emotion peers and asserts `bun -e 'require("./repro.js")'` exits 0 with the expected output.
Fixes #30281. Bumps WebKit to oven-sh/WebKit#217 (preview build `autobuild-preview-pr-217-ae5228e0`). ## What `require()` of an ESM file that statically imports `react` + an MUI v9 forwardRef sub-module (`Typography`, `DialogContent`, etc) aborted on the `m_status == Status::Fetching` assertion at `ModuleRegistryEntry.cpp:254` — SIGABRT on Linux, arm64 PAC IB trap (SIGTRAP) on macOS. Running the same file as the ESM entry (`bun repro.js`) was fine; the bug was on the CommonJS-`require()`-of-ESM path added by #29393. ## Why `moduleRegistryModuleSettled` fired twice for the same `ModuleRegistryEntry`: (1) inline during `hostLoadImportedModule`'s synchronous-replay branch (`fetchPromise` is Fulfilled) where `makeModule` + `fetchComplete` + `modulePromise->fulfillPromise` run synchronously while a `require(esm)` drains the synchronous module queue; (2) from the normal microtask queue for the stale `ModuleRegistryModuleSettled` reaction that `moduleRegistryFetchSettled` had queued *before* the sync mode was entered. `moduleRegistryFetchSettled` already guarded this exact shape with `if (modulePromise->status() != Pending) return`; `moduleRegistryModuleSettled` did not. ## How WebKit-side (oven-sh/WebKit#217): apply the same `modulePromise` pending-guard symmetrically to `moduleRegistryModuleSettled`. Bun-side: `WEBKIT_VERSION` bump + regression test. ## Tests `test/regression/issue/30281.test.ts` installs `react@19` + `@mui/material@9` + the emotion peers and asserts `bun -e 'require("./repro.js")'` exits 0 with the expected output.
Fixes #30281. Bumps WebKit to oven-sh/WebKit#217 (preview build `autobuild-preview-pr-217-ae5228e0`). ## What `require()` of an ESM file that statically imports `react` + an MUI v9 forwardRef sub-module (`Typography`, `DialogContent`, etc) aborted on the `m_status == Status::Fetching` assertion at `ModuleRegistryEntry.cpp:254` — SIGABRT on Linux, arm64 PAC IB trap (SIGTRAP) on macOS. Running the same file as the ESM entry (`bun repro.js`) was fine; the bug was on the CommonJS-`require()`-of-ESM path added by #29393. ## Why `moduleRegistryModuleSettled` fired twice for the same `ModuleRegistryEntry`: (1) inline during `hostLoadImportedModule`'s synchronous-replay branch (`fetchPromise` is Fulfilled) where `makeModule` + `fetchComplete` + `modulePromise->fulfillPromise` run synchronously while a `require(esm)` drains the synchronous module queue; (2) from the normal microtask queue for the stale `ModuleRegistryModuleSettled` reaction that `moduleRegistryFetchSettled` had queued *before* the sync mode was entered. `moduleRegistryFetchSettled` already guarded this exact shape with `if (modulePromise->status() != Pending) return`; `moduleRegistryModuleSettled` did not. ## How WebKit-side (oven-sh/WebKit#217): apply the same `modulePromise` pending-guard symmetrically to `moduleRegistryModuleSettled`. Bun-side: `WEBKIT_VERSION` bump + regression test. ## Tests `test/regression/issue/30281.test.ts` installs `react@19` + `@mui/material@9` + the emotion peers and asserts `bun -e 'require("./repro.js")'` exits 0 with the expected output.
Fixes #30281. Bumps WebKit to oven-sh/WebKit#217 (preview build `autobuild-preview-pr-217-ae5228e0`). ## What `require()` of an ESM file that statically imports `react` + an MUI v9 forwardRef sub-module (`Typography`, `DialogContent`, etc) aborted on the `m_status == Status::Fetching` assertion at `ModuleRegistryEntry.cpp:254` — SIGABRT on Linux, arm64 PAC IB trap (SIGTRAP) on macOS. Running the same file as the ESM entry (`bun repro.js`) was fine; the bug was on the CommonJS-`require()`-of-ESM path added by #29393. ## Why `moduleRegistryModuleSettled` fired twice for the same `ModuleRegistryEntry`: (1) inline during `hostLoadImportedModule`'s synchronous-replay branch (`fetchPromise` is Fulfilled) where `makeModule` + `fetchComplete` + `modulePromise->fulfillPromise` run synchronously while a `require(esm)` drains the synchronous module queue; (2) from the normal microtask queue for the stale `ModuleRegistryModuleSettled` reaction that `moduleRegistryFetchSettled` had queued *before* the sync mode was entered. `moduleRegistryFetchSettled` already guarded this exact shape with `if (modulePromise->status() != Pending) return`; `moduleRegistryModuleSettled` did not. ## How WebKit-side (oven-sh/WebKit#217): apply the same `modulePromise` pending-guard symmetrically to `moduleRegistryModuleSettled`. Bun-side: `WEBKIT_VERSION` bump + regression test. ## Tests `test/regression/issue/30281.test.ts` installs `react@19` + `@mui/material@9` + the emotion peers and asserts `bun -e 'require("./repro.js")'` exits 0 with the expected output.
…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.
…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.
…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.
…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.
… module no longer hangs (#30537) A second `await import()` of a path whose first import threw used to hang forever instead of re-throwing. Two reported variants: - **#23139**: same path twice - **#22743**: path X, then a *different* error path Y, then X again (true regression, 1.2.20 → 1.2.21) While verifying #30527 I checked whether oven-sh/WebKit#225 covered these — turns out both were **already fixed** before that bump (main + WebKit `88b2f7a2` doesn't hang either). The fix landed somewhere in the module-loader rewrite window (#29393 → #30262); the issues just never got re-tested. | build | #23139 | #22743 | |---|---|---| | 1.3.13 | hangs | hangs | | main + WebKit `88b2f7a2` (pre-#30527) | ✅ | ✅ | | main + WebKit `5488984d` | ✅ | ✅ | Both tests fail on `USE_SYSTEM_BUN=1` (stdout missing the post-hang lines, killed by spawn timeout) and pass on `bun bd test`. Fixes #23139 Fixes #22743
Upgrades WebKit to upstream `aac4aed489d1` (2026-04-24) via [oven-sh/WebKit#199](oven-sh/WebKit#199). Supersedes the bisect-revert approach (WebKit oven-sh#190/oven-sh#195) — merges latest upstream/main directly. ## Bun-side changes - **`jsCast<T*>` → `uncheckedDowncast<T>`, `jsDynamicCast<T*>` → `dynamicDowncast<T>`** — 1588 call sites across 248 files + the codegen templates. `jsSecureCast` → `downcast`. 13 redundant same-type casts dropped (new API static_asserts against them). - **`JSScriptFetcher`/`JSScriptFetchParameters` removed** — moduleLoader hooks retyped to `RefPtr<ScriptFetcher>`/`RefPtr<ScriptFetchParameters>`. `loadModuleSync` matches. `scriptFetcher.isObject()` short-circuit removed (old JS-loader quirk; `--eval` result capture keys on the specifier, not fetcher — verified working). - **`JSType.zig`** — two entries removed, 23+ renumbered −2, `EmbedderArrayLike = 237` added. - **NodeVM** — `scriptFetchParametersToImportAttributes()` rebuilds `{type}` for the `importModuleDynamically` callback. - **`WEBKIT_VERSION`** → `autobuild-preview-pr-199-b091462b`. ## Watch for - `Promise.prototype.finally` was split fast/slow upstream; Bun's ALS context wrapping moved to the fast path. Slow path may need coverage if ALS-through-finally tests fail. - `node:vm` `link()` now passes `nullptr` for scriptFetcher (was a JSValue). If node:vm tests rely on threading it, needs a `ScriptFetcher` subclass. --- # WebKit upgrade: d550dd3c808e → aac4aed489d1 565 upstream commits (Apr 16 → Apr 24); 105 touch JSC/WTF/bmalloc. ##⚠️ API/ABI changes that affect Bun bindings ### `jsCast` / `jsDynamicCast` → `downcast` / `dynamicDowncast` migration Upstream is deprecating the JSC-specific cast helpers in favor of WTF's generic `TypeCasts.h`. Bun's C++ bindings use `jsCast` / `jsDynamicCast` heavily — expect deprecation warnings or follow-up migration. - `195397957f97` Make downcast/dynamicDowncast/uncheckedDowncast work with JSCell subclasses - `a6df2880b331` Drop `jsDynamicCast<>()` in favor of `dynamicDowncast<>()` - `a20b2c96bcb4` Drop `jsCast<T*>()` in favor of `uncheckedDowncast<T>()` - `b8ef260b13ef` / `e60220202a25` Reduce remaining `jsCast<>()` uses - `178cea00b798` Replace `jsSecureCast<>()` with `downcast<>()` ### Module loader - `407d0feac1cd` **Remove `JSScriptFetcher` and `JSScriptFetchParameters`** — Bun's loader bridge wraps these; the rerere-resolved files for both headers signal the previous merge already adapted, but verify `ModuleLoader.cpp` - `e236b9dd9455` Fix null-env deref in `CyclicModuleRecord::initializeEnvironment` for Wasm modules ### Header reorganization (drives ~60 of the 75 merge conflicts) Aggressive include-minimization sweep across core JSC headers. Conflicts are mechanical: upstream rewrote include blocks while Bun keeps quote-style `"Foo.h"` instead of `<JavaScriptCore/Foo.h>`. - `9f2eb90301dc` Minimize includes in `CodeBlock.h`, `JSCJSValue.h`, `JSCJSValueInlines.h`, `VM.h` - `744271668d05` Expensive header files slow full build - `5ff0c08af8bf` Reduce cost of `StructureInlines.h` - `405a323e0a80` Use pre-compiled headers consistently for all ports - `4e4c3bd8416a` / `892008ea2763` Missing-include / non-unified-build fixups ## Runtime correctness ### Promise / async - `19dc01a2c79d` Optimize returned promise from async function when it has no `await` - `e704581e3ddb` `Promise.prototype.finally` throws in SpeciesConstructor before `then` - `13b3b3095f1e` [Win] Add missing `SYSV_ABI` to `pinballHandlerFinishReject` ### Arrays / TypedArrays / Objects - `286f7a92228f` Add `JSArray::fastShift` - `4c82252b8b2f` Snapshot span of TypedArrays in `toSorted`/`toReversed`/`with` - `cdd970b5238b` Fast path for `Array.prototype.indexOf` on NodeList - `8eca46ec5bfb` Fix `Object.defineProperties` Proxy trap ordering - `9b1a02808762` Fix megamorphic IC ownProperty check - `7eb28729693c` Correct IDL union handling of ArrayBuffer & friends ### Strings / JSON / BigInt - `128591e63775` Cache `JSString` cells for short `JSON.parse` string values - `fa83cf53f871` Avoid rope resolution for single-char `startsWith`/`endsWith` - `8f585979f1b4` Clean up `JSBigInt::cachedMod` ### Intl - `e6fae9eeaf4e` Introduce Canonicalized TimeZone mechanism - `239255f394a2` `Intl.DateTimeFormat` keep original legacy `[[TimeZone]]` - `367f77ea6640` Optimize `Intl.NumberFormat` creation - `7e5d9f982d7b` Report external memory in `Intl.NumberFormat`/`PluralRules` - `325456987d1d` `Intl.Locale` canonicalize before override language - `0145d169cd9c` `Intl.Segmenter` `isWordLike` off-by-one fix ### YARR / RegExp - `7c8c020f78ce` Fix YarrJIT SIGBUS from sibling-alt frame-slot aliasing - `282d55d7a141` FixedCount ParenContext: mark incomplete and retry - `062fabff2067` Unicode-mode non-capturing groups backreference count - `582f25b3e697` Forward references no longer JIT-compiled ## JIT / DFG / FTL / register allocator - `fa2381cad819` Use runtime indexing type for write barrier in `operationPopulateObjectInOSR` - `289a55e3913b` Fix hole-handling when rematerializing sunk double arrays - `460528ee4187` Escape `MultiGetByOffset` constants not convertible to double - `020b68a05dfa` Run `lowerAfterRegAlloc` before `fixObviousSpills` - `b8d759043c88` Lower `BitAnd(SShr(x,lsb),mask)` → `ubfx` on ARM64 - `bc02c09f91bb` Refine clobberize rule for `ArrayIsArray` - `0b634d738806` / `173d202e5943` / `86d3f77f318b` GreedyRegAlloc refactors (split metadata, typed costs, per-fn dump) - `51024b1e8ca6` Don't assert on self comparison in `buildCoalescingGroups` - `be7c61dd4afc` Fix `!ENABLE(JIT)` build - `7999f4e1693b` Disable ARM64E dotprod ## WebAssembly - `85da494b1c29` Implement relaxed Wasm SIMD - `7a12263d0ac4` / `88fb9a875ac5` / `14c9a4cfcf8a` Memory64: atomics, bulk memory, grow/size in OMG - `e40405afe9a4` / `98a27e9f7485` Multimemory: data segments + post-land refactor - `787be9470e19` BBQ: fix integer div/rem with `INT_MIN`/-1 - `a03517078f7e` BBQ: fp min/max negative-zero handling - `e07d187aa4dd` BBQ JIT gets stack size from IPInt - `c628c3c9d8ee` Crash on wide-arithmetic instructions - `1857337a10a5` Simplify IPInt local accessors - `a0d2715d2060` `WebAssemblyGCStructureTypeDependencies` traverse expanded types - `f45de3a3d2a7` / `bdb1b12f3a60` / `13c5aeda0703` / `7d074dda6cbe` Wasm Debugger: reset deadlock, null stopData, remove `stop()`, ordering bugs ## WTF - `2726e5dbb0cf` Remove runtime support for older Apple OS versions (touches `PlatformHave.h` heavily) - `21ac6cec98ee` Remove support for SDKs older than 26.2 - `0d04522925ea` Use `std::span` SIMDUTF API - `d814bd4d3a84` Add `RetainRef` (non-nullable `RetainPtr`) - `3b05441afa50` `Vector::grow()` assert → release assert - `1ae1abc47aac` Fix TSAN false-positive on `ThreadSafeWeakPtrControlBlock` - `19570fa4f1cb` `WTF::span(char**)` returns empty span for null - `98bb7482cf59` [MTE] Harden `ThreadTimerHeapItem` / `CompactPointerTuple` - `cdf397cab58d` Partial revert of `WTFCrashWithInfo` Linux/Armv7 - `f6c0c3a4c8ed` Reduce C-style arrays ## bmalloc / libpas - `1b1e4d08c01c` Fix initialization check for LDM state - `d7b3180cbfe8` Fix libpas standalone build ## Platform / build - `fc480fe9b885` Set ARM64 CPU feature bits on Windows - `2e7c3456cd6d` [Win] Cross-compile ARM64 from Linux - `3d242bc9a178` Use `UncheckedKeyHashMap`/`UncheckedKeyHashSet` - `92526e91925a` Suppress `-Wunsafe-buffer-usage` warnings - Reverts: `c63a29174071`, `7b7926d28328`, `550e10f0c386`, `75f92ada1361` ## Watch list for Bun - **`jsCast`/`jsDynamicCast` removal** — Bun bindings will need migration (or rely on the compat shims if upstream left any) - **`JSScriptFetcher`/`JSScriptFetchParameters` removed** — Bun's `ModuleLoader.cpp` and `BakeSourceProvider.cpp` reference these - **`PlatformHave.h` SDK-floor bump** — 32 conflict hunks; Bun's `OS(DARWIN) && USE(BUN_JSC_ADDITIONS)` overrides need re-threading - **Promise no-await optimization (`19dc01a2c79d`)** — interacts with Bun's microtask queue customizations in `JSMicrotask.cpp` - **`Vector::grow()` release assert** — any over-reserve in Bun bindings now fatal in release --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
…9738) Follow-up to oven-sh#29719. `bun:main` statically imports the entry file, so `await import("bun:main")` at the entry's top level is a TLA self-cycle: `bun:main` waits for `entry.mjs` (async dep), and `entry.mjs` waits for `bun:main`'s evaluation promise. Per spec that promise never settles. The old JSC module loader broke these cycles early, which is why oven-sh#29719 passed locally (tested against `892042c2`, pre-oven-sh#29393). After oven-sh#29393 (WebKit module-loader rewrite) the loader correctly leaves the promise unsettled, so the test now hangs and times out at 90s — see build 48023 (debian-asan, win x64, win x64-baseline). Fix: drop the top-level `await` and use `import("bun:main").then(...)` so `entry.mjs` finishes synchronously, `bun:main` finishes, and the import resolves on the next microtask. The preload and `--hot` tests are unaffected (preload isn't in `bun:main`'s dep graph). Note: this also surfaced that Bun now hangs forever on any unsettled-TLA cycle instead of exiting 13 like Node — separate PR coming for that. ## How did you verify your code works? - `bun bd test test/js/bun/resolve/bun-main-entry-point.test.ts` — 3 pass, 20 consecutive runs on Windows - `USE_SYSTEM_BUN=1 bun test ...` — 2 fail (still catches the alias bug on unfixed bun)
…o 100ms) (oven-sh#29741) ## What does this PR do? Deflakes `test/cli/run/cpu-prof.test.ts` on Windows, which flaked in 9 of the last 200 builds — e.g. [build 48023](https://buildkite.com/bun/bun/builds/48023) on Windows 2019 x64-baseline at line 187: ``` expect(functionNames.some((name: string) => name !== "(root)" && name !== "(program)")).toBe(true); Expected: true Received: false ``` and again on this PR's first push ([build 48042](https://buildkite.com/bun/bun/builds/48042)) at lines 241/285 with `Received: "No samples collected.\n"`. **Root cause:** On Windows, JSC's SamplingProfiler effectively ticks at the ~15.6ms default timer quantum (WTF::sleep is bounded by it without `timeBeginPeriod`). After oven-sh#29393 (WebKit module-loader rewrite), entry-module evaluation is async — `loadAndEvaluateModule` returns a pending promise and user code runs after fetch→link→evaluate microtask checkpoints — so the first sampler tick can land in loader internals before user code starts. The previous workloads were either iteration-bounded (`for (i < 1000000)`, JITs to <1ms) or time-bounded for only 32/50ms, which is no longer enough margin to guarantee even one sample lands in user code. 7 of the 9 historical flakes are post-oven-sh#29393; the other 2 predate it. The test was always borderline, the rewrite made it ~7× flakier. Jarred's earlier deflake (8058d78) bumped 16→32ms and time-bounded the first test, but that's no longer sufficient. **Fix:** Time-bound every workload in the file for 100ms (~6 sampler ticks on Windows), including the previously iteration-bounded `myFunction()`. Under `describe.concurrent` the wall-clock cost is the max not the sum, so the suite stays at ~1.9s. ## How did you verify your code works? - `bun bd test test/cli/run/cpu-prof.test.ts` — 30/30 consecutive passes on Windows after each commit - BuildKite history scan of last 200 builds correlating flake commits with WebKit upgrade ancestry
…n 1.4.0 GA (#2827) Re-diagnosed the slice 6 blocker as a 1.3.14 TLA-under-`--isolate` TDZ regression (not a `mock.module()` issue). Filed oven-sh/bun#31410, bisected to `73e8889f8c` (WebKit module-loader rewrite, oven-sh/bun#29393), verified fixed on bun canary `1.4.0-canary.1+0974d031c` via oven-sh/bun#30656 + oven-sh/WebKit#230. Slice 6 cutover (#2802) now waits on bun 1.4.0 GA so the engine pin lifts in the same PR. Wall-clock plan posted on #2802: keep the 4-shard matrix (collapsing is slower than the current 95s slowest-shard critical path), add `--changed=origin/main` for PR runs (~95s → ~20s), duration-balanced shard partitioning, native `--parallel` worker pool. Refs #2811 (closed), #2802, #2796.
4 participants
Upgrades WebKit to upstream
aac4aed489d1(2026-04-24) via oven-sh/WebKit#199.Supersedes the bisect-revert approach (WebKit #190/#195) — merges latest upstream/main directly.
Bun-side changes
jsCast<T*>→uncheckedDowncast<T>,jsDynamicCast<T*>→dynamicDowncast<T>— 1588 call sites across 248 files + the codegen templates.jsSecureCast→downcast. 13 redundant same-type casts dropped (new API static_asserts against them).JSScriptFetcher/JSScriptFetchParametersremoved — moduleLoader hooks retyped toRefPtr<ScriptFetcher>/RefPtr<ScriptFetchParameters>.loadModuleSyncmatches.scriptFetcher.isObject()short-circuit removed (old JS-loader quirk;--evalresult capture keys on the specifier, not fetcher — verified working).JSType.zig— two entries removed, 23+ renumbered −2,EmbedderArrayLike = 237added.scriptFetchParametersToImportAttributes()rebuilds{type}for theimportModuleDynamicallycallback.WEBKIT_VERSION→autobuild-preview-pr-199-b091462b.Watch for
Promise.prototype.finallywas split fast/slow upstream; Bun's ALS context wrapping moved to the fast path. Slow path may need coverage if ALS-through-finally tests fail.node:vmlink()now passesnullptrfor scriptFetcher (was a JSValue). If node:vm tests rely on threading it, needs aScriptFetchersubclass.WebKit upgrade: d550dd3c808e → aac4aed489d1
565 upstream commits (Apr 16 → Apr 24); 105 touch JSC/WTF/bmalloc.
jsCast/jsDynamicCast→downcast/dynamicDowncastmigrationUpstream is deprecating the JSC-specific cast helpers in favor of WTF's generic
TypeCasts.h. Bun's C++ bindings usejsCast/jsDynamicCastheavily — expect deprecation warnings or follow-up migration.195397957f97Make downcast/dynamicDowncast/uncheckedDowncast work with JSCell subclassesa6df2880b331DropjsDynamicCast<>()in favor ofdynamicDowncast<>()a20b2c96bcb4DropjsCast<T*>()in favor ofuncheckedDowncast<T>()b8ef260b13ef/e60220202a25Reduce remainingjsCast<>()uses178cea00b798ReplacejsSecureCast<>()withdowncast<>()Module loader
407d0feac1cdRemoveJSScriptFetcherandJSScriptFetchParameters— Bun's loader bridge wraps these; the rerere-resolved files for both headers signal the previous merge already adapted, but verifyModuleLoader.cppe236b9dd9455Fix null-env deref inCyclicModuleRecord::initializeEnvironmentfor Wasm modulesHeader reorganization (drives ~60 of the 75 merge conflicts)
Aggressive include-minimization sweep across core JSC headers. Conflicts are mechanical: upstream rewrote include blocks while Bun keeps quote-style
"Foo.h"instead of<JavaScriptCore/Foo.h>.9f2eb90301dcMinimize includes inCodeBlock.h,JSCJSValue.h,JSCJSValueInlines.h,VM.h744271668d05Expensive header files slow full build5ff0c08af8bfReduce cost ofStructureInlines.h405a323e0a80Use pre-compiled headers consistently for all ports4e4c3bd8416a/892008ea2763Missing-include / non-unified-build fixupsRuntime correctness
Promise / async
19dc01a2c79dOptimize returned promise from async function when it has noawaite704581e3ddbPromise.prototype.finallythrows in SpeciesConstructor beforethen13b3b3095f1e[Win] Add missingSYSV_ABItopinballHandlerFinishRejectArrays / TypedArrays / Objects
286f7a92228fAddJSArray::fastShift4c82252b8b2fSnapshot span of TypedArrays intoSorted/toReversed/withcdd970b5238bFast path forArray.prototype.indexOfon NodeList8eca46ec5bfbFixObject.definePropertiesProxy trap ordering9b1a02808762Fix megamorphic IC ownProperty check7eb28729693cCorrect IDL union handling of ArrayBuffer & friendsStrings / JSON / BigInt
128591e63775CacheJSStringcells for shortJSON.parsestring valuesfa83cf53f871Avoid rope resolution for single-charstartsWith/endsWith8f585979f1b4Clean upJSBigInt::cachedModIntl
e6fae9eeaf4eIntroduce Canonicalized TimeZone mechanism239255f394a2Intl.DateTimeFormatkeep original legacy[[TimeZone]]367f77ea6640OptimizeIntl.NumberFormatcreation7e5d9f982d7bReport external memory inIntl.NumberFormat/PluralRules325456987d1dIntl.Localecanonicalize before override language0145d169cd9cIntl.SegmenterisWordLikeoff-by-one fixYARR / RegExp
7c8c020f78ceFix YarrJIT SIGBUS from sibling-alt frame-slot aliasing282d55d7a141FixedCount ParenContext: mark incomplete and retry062fabff2067Unicode-mode non-capturing groups backreference count582f25b3e697Forward references no longer JIT-compiledJIT / DFG / FTL / register allocator
fa2381cad819Use runtime indexing type for write barrier inoperationPopulateObjectInOSR289a55e3913bFix hole-handling when rematerializing sunk double arrays460528ee4187EscapeMultiGetByOffsetconstants not convertible to double020b68a05dfaRunlowerAfterRegAllocbeforefixObviousSpillsb8d759043c88LowerBitAnd(SShr(x,lsb),mask)→ubfxon ARM64bc02c09f91bbRefine clobberize rule forArrayIsArray0b634d738806/173d202e5943/86d3f77f318bGreedyRegAlloc refactors (split metadata, typed costs, per-fn dump)51024b1e8ca6Don't assert on self comparison inbuildCoalescingGroupsbe7c61dd4afcFix!ENABLE(JIT)build7999f4e1693bDisable ARM64E dotprodWebAssembly
85da494b1c29Implement relaxed Wasm SIMD7a12263d0ac4/88fb9a875ac5/14c9a4cfcf8aMemory64: atomics, bulk memory, grow/size in OMGe40405afe9a4/98a27e9f7485Multimemory: data segments + post-land refactor787be9470e19BBQ: fix integer div/rem withINT_MIN/-1a03517078f7eBBQ: fp min/max negative-zero handlinge07d187aa4ddBBQ JIT gets stack size from IPIntc628c3c9d8eeCrash on wide-arithmetic instructions1857337a10a5Simplify IPInt local accessorsa0d2715d2060WebAssemblyGCStructureTypeDependenciestraverse expanded typesf45de3a3d2a7/bdb1b12f3a60/13c5aeda0703/7d074dda6cbeWasm Debugger: reset deadlock, null stopData, removestop(), ordering bugsWTF
2726e5dbb0cfRemove runtime support for older Apple OS versions (touchesPlatformHave.hheavily)21ac6cec98eeRemove support for SDKs older than 26.20d04522925eaUsestd::spanSIMDUTF APId814bd4d3a84AddRetainRef(non-nullableRetainPtr)3b05441afa50Vector::grow()assert → release assert1ae1abc47aacFix TSAN false-positive onThreadSafeWeakPtrControlBlock19570fa4f1cbWTF::span(char**)returns empty span for null98bb7482cf59[MTE] HardenThreadTimerHeapItem/CompactPointerTuplecdf397cab58dPartial revert ofWTFCrashWithInfoLinux/Armv7f6c0c3a4c8edReduce C-style arraysbmalloc / libpas
1b1e4d08c01cFix initialization check for LDM stated7b3180cbfe8Fix libpas standalone buildPlatform / build
fc480fe9b885Set ARM64 CPU feature bits on Windows2e7c3456cd6d[Win] Cross-compile ARM64 from Linux3d242bc9a178UseUncheckedKeyHashMap/UncheckedKeyHashSet92526e91925aSuppress-Wunsafe-buffer-usagewarningsc63a29174071,7b7926d28328,550e10f0c386,75f92ada1361Watch list for Bun
jsCast/jsDynamicCastremoval — Bun bindings will need migration (or rely on the compat shims if upstream left any)JSScriptFetcher/JSScriptFetchParametersremoved — Bun'sModuleLoader.cppandBakeSourceProvider.cppreference thesePlatformHave.hSDK-floor bump — 32 conflict hunks; Bun'sOS(DARWIN) && USE(BUN_JSC_ADDITIONS)overrides need re-threading19dc01a2c79d) — interacts with Bun's microtask queue customizations inJSMicrotask.cppVector::grow()release assert — any over-reserve in Bun bindings now fatal in release