fix(windows): disable bmalloc/libpas linking to fix crash#28098
Open
fix(windows): disable bmalloc/libpas linking to fix crash#28098
Conversation
bmalloc/libpas has a documented history of instability on Windows, causing crashes during garbage collection under memory pressure. It was originally disabled in PR #20931, accidentally re-enabled in PR #26381 (WebKit update), and fixed in commits 91949f7/9b87e4a3f3 — but those fixes were never merged to main. The recent CMake-to-TypeScript build migration also carried over the unconditional bmalloc linking. This removes bmalloc.lib from Windows link targets in both the legacy CMake build and the new TypeScript (ninja) build system. Fixes #28097 Fixes #26985 Fixes #26982 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Updated 4:51 AM PT - Mar 14th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 28098That installs a local version of the PR into your bun-28098 --bun |
Contributor
WalkthroughAdded inline documentation comments to WebKit dependency paths noting that bmallocLib should be excluded on Windows once prebuilts are rebuilt with Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 262-264: Add a CI regression guard that fails on Windows if
bmalloc/libpas get into the link inputs: after the libs array is computed (the
variable libs built from coreLibs(cfg), prebuiltIcuLibs(cfg), and
bmallocLib(cfg)) and likewise after the other final-link-inputs block around
lines 287-290 where link inputs are assembled, add a check when cfg.windows is
true that scans the combined input paths and throws/returns a hard failure (or
process.exit(1)) if any entry contains "bmalloc" or "libpas" (or matches
"bmalloc.lib"/"libpas" filenames); reference the symbols libs, coreLibs,
prebuiltIcuLibs, bmallocLib and the final link-input array and fail with a clear
message so CI catches any accidental reintroduction on Windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3cc2fe1a-dacc-427a-b413-7f5e3bb48db6
📒 Files selected for processing (2)
cmake/targets/BuildBun.cmakescripts/build/deps/webkit.ts
💤 Files with no reviewable changes (1)
- cmake/targets/BuildBun.cmake
Adds assertNoBmallocOnWindows() that throws a clear error if any bmalloc or libpas library sneaks into the Windows link inputs. Called in both the prebuilt and local build paths of the provides() function so CI catches any accidental reintroduction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Found 8 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The prebuilt WebKit for Windows is currently compiled without -DUSE_SYSTEM_MALLOC=ON, so WTF.lib and JavaScriptCore.lib have references to bmalloc symbols. Simply removing bmalloc.lib from the link inputs causes unresolved symbol errors. The proper fix requires two coordinated changes: 1. oven-sh/WebKit: Re-add -DUSE_SYSTEM_MALLOC=ON to windows-release.ps1 (was removed after WebKit PR #97 added it, but a later update dropped it) 2. oven-sh/bun: Remove bmalloc.lib from Windows link targets This commit adds TODO(#28097) comments marking where bmalloc should be excluded once the prebuilts are rebuilt. Refs #28097 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 261-265: The bmalloc/libpas allocator is still being appended
unconditionally; update the arrays that include bmallocLib(cfg) (the lines that
build const libs = [...coreLibs(cfg), ...prebuiltIcuLibs(cfg), bmallocLib(cfg)]
and the similar prebuilt/local path around the other occurrence) to exclude
bmallocLib when building for Windows by checking the cfg platform (e.g., if
(cfg.platform !== 'windows') push bmallocLib(cfg) or use a conditional spread).
Ensure both places (the prebuilt path and the local/core libs assembly where
bmallocLib(cfg) is currently added) use the same platform guard so Windows
builds do not link bmalloc/libpas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37fbb172-c005-46e6-90cb-b4fae5a070c0
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
Comment on lines
+261
to
265
| // | ||
| // TODO(#28097): bmalloc/libpas causes crashes on Windows under memory | ||
| // pressure. Once oven-sh/WebKit rebuilds prebuilts with | ||
| // -DUSE_SYSTEM_MALLOC=ON, exclude bmallocLib on Windows here. | ||
| const libs = [...coreLibs(cfg), ...prebuiltIcuLibs(cfg), bmallocLib(cfg)]; |
Contributor
There was a problem hiding this comment.
TODO-only update leaves the Windows crash path unchanged.
Line 265 and Line 290 still append bmallocLib(cfg) unconditionally, so Windows continues linking bmalloc/libpas in both prebuilt and local paths. That means the allocator-crash mitigation objective is not actually implemented yet. This needs a real Windows exclusion (or the prerequisite WebKit/prebuilt allocator switch delivered in the same PR) before merge.
Also applies to: 288-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build/deps/webkit.ts` around lines 261 - 265, The bmalloc/libpas
allocator is still being appended unconditionally; update the arrays that
include bmallocLib(cfg) (the lines that build const libs = [...coreLibs(cfg),
...prebuiltIcuLibs(cfg), bmallocLib(cfg)] and the similar prebuilt/local path
around the other occurrence) to exclude bmallocLib when building for Windows by
checking the cfg platform (e.g., if (cfg.platform !== 'windows') push
bmallocLib(cfg) or use a conditional spread). Ensure both places (the prebuilt
path and the local/core libs assembly where bmallocLib(cfg) is currently added)
use the same platform guard so Windows builds do not link bmalloc/libpas.
Collaborator
Author
There was a problem hiding this comment.
I initially made this exact change (excluding bmalloc on Windows), but CI build #39584 failed on windows-x64-build-bun with link errors.
Root cause: The prebuilt WebKit for Windows (commit 00e825523d) is compiled without -DUSE_SYSTEM_MALLOC=ON, so WTF.lib and JavaScriptCore.lib contain unresolved references to bmalloc symbols. We can't drop bmalloc.lib from the link inputs until the prebuilts are rebuilt.
The fix that worked in July 2025 (Bun PR #20931) was paired with WebKit PR #97 which added -DUSE_SYSTEM_MALLOC=ON to windows-release.ps1. A later WebKit update dropped that flag.
Tracked in #28101 — once the upstream prebuilts are rebuilt with -DUSE_SYSTEM_MALLOC=ON, a follow-up PR can:
- Exclude
bmallocLib(cfg)on Windows (where the TODO comments are now) - Remove
bmalloc.libfromBuildBun.cmake - Add the
assertNoBmallocOnWindowsCI guard
Contributor
There was a problem hiding this comment.
@robobun, understood — thanks for the detailed explanation. The link failure confirms the constraint: until WTF.lib / JavaScriptCore.lib in the Windows prebuilts are rebuilt with -DUSE_SYSTEM_MALLOC=ON, the bmalloc symbol references are baked into those archives and bmalloc.lib must stay on the link line. The TODO comments with the #28097 / #28101 backlinks are the correct interim approach here. I'll withdraw my earlier critique.
✏️ Learnings added
Learnt from: robobun
Repo: oven-sh/bun PR: 28098
File: scripts/build/deps/webkit.ts:261-265
Timestamp: 2026-03-14T09:39:04.350Z
Learning: In oven-sh/bun, the prebuilt WebKit for Windows (as of commit 00e825523d) is built WITHOUT -DUSE_SYSTEM_MALLOC=ON, so WTF.lib and JavaScriptCore.lib contain unresolved references to bmalloc symbols. Dropping bmallocLib(cfg) on Windows in scripts/build/deps/webkit.ts causes link errors until the upstream oven-sh/WebKit prebuilts are rebuilt with that flag. The two-step fix (tracked in PR `#28101`) is: (1) rebuild prebuilts with -DUSE_SYSTEM_MALLOC=ON in windows-release.ps1, then (2) exclude bmallocLib on Windows and remove bmalloc.lib from cmake/targets/BuildBun.cmake. Historical reference: this was fixed in bun PR `#20931` paired with WebKit PR `#97`, but a later WebKit update dropped the flag and PR `#26381` reintroduced unconditional bmalloc linking.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time
Learnt from: alii
Repo: oven-sh/bun PR: 26402
File: src/bundler/linker_context/generateChunksInParallel.zig:307-355
Timestamp: 2026-01-23T22:52:42.507Z
Learning: In Bun's bundler linker context (src/bundler/linker_context/*.zig), module_info operations for ESM bytecode caching use `catch continue` / `catch {}` to silently handle allocation failures because module_info is supplementary metadata, not critical to producing correct JavaScript output. Critical allocations use `bun.handleOom()` or `try`, but module_info operations (mi.str(), mi.addVar(), mi.requestModule(), mi.addImportInfoSingle(), etc.) intentionally use graceful degradation to avoid crashing builds on OOM for non-critical metadata. This pattern is used consistently across postProcessJSChunk.zig, generateChunksInParallel.zig, and related files.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: scripts/verify-baseline-static/CLAUDE.md:0-0
Timestamp: 2026-03-11T09:35:29.976Z
Learning: Applies to scripts/verify-baseline-static/**/allowlist-*-windows.txt : For Windows PDB symbol forms that fall back to linker attribution, allowlist entries using the form `<lib:NAME.lib>` to capture section-contribution data stable across link layout changes
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:60-76
Timestamp: 2025-09-02T17:41:07.869Z
Learning: In bun's memory utilities, when handling const pointers in deinit operations, prefer compile-time errors over silent skipping to avoid hard-to-find memory leaks. Users expect explicit failures rather than silent omissions in memory management.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: scripts/verify-baseline-static/CLAUDE.md:0-0
Timestamp: 2026-03-11T09:35:29.976Z
Learning: Applies to scripts/verify-baseline-static/**/{allowlist-*.txt,allowlist*.txt} : For Rust v0 mangled symbols in allowlist entries, use `<rust-hash>` in place of the crate-hash (e.g., `Cs[base62]_`) to survive toolchain bumps
Learnt from: robobun
Repo: oven-sh/bun PR: 27056
File: test/bundler/standalone.test.ts:281-324
Timestamp: 2026-02-16T04:26:34.633Z
Learning: In Bun's test files for `Bun.build()`, configuration validation errors (such as invalid targets, missing entrypoints, or incompatible option combinations) are thrown synchronously from `JSBundler.fromJS()` via `globalThis.throwInvalidArguments()` before a Promise is returned. These should be tested with `expect(() => Bun.build({...})).toThrow()`, not `await expect(...).rejects.toThrow()`. The Promise rejection pattern is only for errors that occur during the async build process itself.
<!-- </add_learning>
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:41.142Z
Learning: In Bun's C++ bindings (src/bun.js/bindings/helpers.h and similar files), when returning an empty JSC::Identifier and a VM is accessible, prefer using `vm.propertyNames->emptyIdentifier` over constructing with `JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier)`. The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns.
Collaborator
Author
Why bmalloc.lib can't be removed yetI initially removed The prebuilt WebKit for Windows (commit History
Required fixThe proper fix is a two-step coordinated change:
This PR adds TODO comments marking where the changes should go once step 1 is complete. |
This was referenced Mar 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bmalloc/libpas causes crashes on Windows under memory pressure during garbage collection (10GB+ RSS). This was previously fixed in WebKit PR #97 (
-DUSE_SYSTEM_MALLOC=ON) and Bun PR #20931 (removingbmalloc.libfrom link targets), but a later WebKit update dropped theUSE_SYSTEM_MALLOC=ONflag fromwindows-release.ps1, re-enabling bmalloc in the prebuilt binaries.Root cause
The prebuilt WebKit for Windows (commit
00e825523d) is compiled without-DUSE_SYSTEM_MALLOC=ON, soWTF.libandJavaScriptCore.libcontain references to bmalloc symbols. Simply removingbmalloc.libfrom the Bun link step causes unresolved symbol errors (CI build #39584 failed onwindows-x64-build-bun).What this PR does
Adds
TODO(#28097)comments inscripts/build/deps/webkit.tsmarking where bmalloc should be excluded on Windows once the upstream fix is applied.Required upstream fix (tracked in #28101)
The full fix requires a coordinated change:
-DUSE_SYSTEM_MALLOC=ONtowindows-release.ps1and publish new prebuiltsbmalloc.libfrom Windows link targets, excludebmallocLib(cfg)on Windows, add CI regression guardRelated issues
🤖 Generated with Claude Code