Repository navigation
Conversation
The CSS linker cloned each @import condition into arena storage that runs no destructor, and cloned it again for the @supports and @media wrapper rules of a chunk. A condition can own heap memory: the fallback or the indices of env(), a calc() tree, a layer name with more than one segment. Each clone leaked that memory. LeakSanitizer reports it for `@import "./b.css" (min-width: env(foo, 1px));`. CssImportOrder.conditions is now a list of references to the conditions of each parsed ImportRule. The list is in the LinkerGraph arena and is Copy, so CssImportOrder needs no Drop impl. The wrapper rules take a bitwise copy, as the @layer wrapper already did. The parsed stylesheet stays the only owner and frees the memory once, at bundle teardown. ImportRule now has a conditions field. The accessor that cast between ImportRule and ImportConditions, the repr(C) attributes and the layout assertions are gone. Also removed: deep_clone_conditions, ImportRule::conditions_with_import_records, the clone_with_import_records functions of LayerName, SupportsCondition and MediaList, and the import-records list that Visitor::visit passed down and nothing wrote. Tests: spawned builds with leak checking on, the same fixtures through Bun.build, two CSS chunks that share a stylesheet, and an RSS check for builds without ASAN.
The dev server links the import chain again on every rebuild. The new case has a conditional import under a conditional import, edits the condition of the outer file and then the leaf, and asserts the served CSS at each step. On an ASAN build the dev server process runs with leak checking on, and with BUN_DESTRUCT_VM_ON_EXIT so that memory the VM still owns is not reported. Without the fix it exits with a LeakSanitizer report for the cloned conditions.
|
Status Closed: #46021 fixed this leak on main. How the leak was reproduced, on main at echo '.b1 { color: green }' > b.css
printf '@import "./b.css" (min-width: env(foo, 1px));\n.c0 { color: blue }\n' > entry.css
ASAN_OPTIONS=detect_leaks=1 bun-debug build ./entry.css --outdir outLeakSanitizer reported |
|
Updated 6:05 AM PT - Oct 20th, 2026
❌ @robobun, your commit 6bf4d6f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46176That installs a local version of the PR into your bun-46176 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughCSS import conditions are consolidated in ChangesCSS import condition handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: borrowing CSS @import conditions from parsed stylesheets. |
| Description check | |
The description thoroughly explains the problem, fix, rationale, risks, and verification results. It does not use the exact template headings, but it provides all required information and is complete. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the two soundness questions the borrow raises and ruled them out: the BackRef<ImportConditions> lists are only read while the parsed stylesheets are alive (chunks drop inside the build closure before deinit_without_freeing_arena, and the dev server drops chunks in finalize_bundle before finalize_bundle_cleanup), and the bitwise-copied @ media/@ supports wrapper rules go into arena rule slabs that CssChunk::drop empties with set_len(0), so no second Drop runs on the aliased heap.
Extended reasoning...
The change rewires the CSS linker (findImportedFilesInCSSOrder.rs, prepareCssAstsForChunk.rs, Chunk.rs) and the css crate's ImportRule to store non-owning BackRef pointers into parsed stylesheets instead of deep clones in arena slabs, removing a Drop impl and several clone helpers; it touches no security-sensitive surface. It is unsafe-Rust lifetime work with one confirmed inline finding, so a human review is warranted; the ruled-out items above were verified against bundle_v2.rs and dev_server/finalize.rs ordering.
|
Closing: #46021 fixed this leak on main. #46021 replaces the deep clones of the I checked main at
What this PR still adds to main is a refactor and tests, not a fix:
The branch stays. Reopen this PR if a maintainer wants the refactor or the tests. |
1 participant
Problem
bun buildaborts when an@importcondition owns heap memory:Direct leak of 144 byte(s) in 2 object(s)in<TokenList as DeepClone>::deep_clonefor@import "./b.css" (min-width: env(foo, 1px));.env(),calc()or a dotted layer name leaks 16 to 704 bytes per build. No user reported it.Drop(findImportedFilesInCSSOrder.rs:187,prepareCssAstsForChunk.rs:542).Fix
CssImportOrder.conditionsnow lists references to the newconditionsfield of each parsedImportRule. The linker clones no condition.@supportsand@mediawrappers take a bitwise copy, as the@layerwrapper did. The parsed stylesheet stays the only owner.test/bundler/esbuild/css.test.ts(10 new) andtest/bake/dev/css.test.ts(1) fail on an ASAN build of main and pass here. Also rantest/bundler/css/*.test.ts,bundler_html.test.tsandtest/js/bun/css/css.test.ts.Background
layer(),supports()and media query of an@import. The bundler wraps inlined rules in them.CssImportOrderentry. Fix effectively every native-code memory leak in Bun #30875 put its condition list in the linker arena, which runs no destructor.Vec<ImportConditions>, a registry of clones, and arena-backed value types. Borrowing removes the clones and theDropthat forgets the list.Downsides
url()in anenv()fallback of an import condition still panics when printed, as on main. css: keep a url() in an at-rule prelude as written, and never print the resolved path of a url() #45604 fixes it.Notes
Reproduce
On main the report has two allocations of 72 bytes. One is in
Visitor::visit, throughImportRule::conditions_with_import_records(findImportedFilesInCSSOrder.rs:187). The other is inwrap_rules_with_conditions, throughMediaList::clone_with_import_records(prepareCssAstsForChunk.rs:542). The CI lane keeps 30 stack frames. At that depth the two stacks are equal, and LeakSanitizer prints one entry of 144 bytes.Cause
Visitor::visitmade aVec<ImportConditions>for each conditional import. It cloned the wrapping conditions into a slab in theLinkerGrapharena (deep_clone_conditions,findImportedFilesInCSSOrder.rs:747), then cloned the conditions of the rule.CssImportOrder::dropforgot the list. No code ranDropfor a clonedImportConditions.wrap_rules_with_conditionsclonedsupportsandmediaagain into wrapper rules. Those rules are in an arena rule list, andCssChunk::dropfrees that list withset_len(0).indicesand thefallbackof anEnvironmentVariable, theBoxof aCalc, and the segments of aLayerNamewith more than one segment.Why the borrow is sound
deinit_without_freeing_arena(src/bundler/bundle_v2.rs:5541). The chunks and their order lists are dropped before that. The dev server drops the chunks infinalize_bundle(src/runtime/bake/dev_server/finalize.rs:215), then callsfinalize_bundle_cleanup.LinkerGrapharena, which lives as long as the AST table.RawSliceandBackRefareCopyand have no destructor. The linker can copy an order entry bitwise, truncate its list, or discard it. Nothing is freed twice and nothing leaks.conditionsfield of anImportRulein therulesbuffer of a parsed stylesheet. After the parse, the linker andbundle_v2.rsread that buffer through shared references. The only mutable access found is the teardown drop.What the diff removes
deep_clone_conditions,shallow_clone_records,ImportRule::conditions_with_import_records, and theclone_with_import_recordsfunctions ofLayerName,SupportsConditionandMediaList.impl Drop for CssImportOrder.ImportRule::conditions_mut, therepr(C)attributes and the layout assertions that made its cast valid, and the second copy of the condition printer inImportRule::to_css.wrapping_import_recordsparameter ofVisitor::visit. Nothing wrote to that list, so it was always empty.unsafetokens in the 8 changed source files: 81 to 75.Tests
css/CSSAtImportConditionsLeakCheck{Env,Calc,External,Nested}: a spawnedbun buildwith leak checking on, and the exact output. On a debug build of main each fails with a LeakSanitizer summary: 704, 2,112, 256 and 744 bytes.APIsuffix runBun.buildin the test process and check the output. On main under the CI lane settings, that process aborts at exit with3816 byte(s) leaked in 107 allocation(s).css/CSSAtImportConditionsLeakCheckTwoChunks: three entry points give two CSS chunks that wrap one stylesheet in the same condition. Main leaks 360 bytes.css/CSSAtImportConditionsLeakCheckRSSruns only on builds without ASAN. It callsBun.build60 times on a condition with a longenv()fallback. The bound is 100 MiB. The released canary (367d939d9) grows 367 MiB. A release build of this branch grows 3.7 to 8.1 MiB over 6 runs.test/bake/dev/css.test.ts,nested conditional css imports across hot reloads: it edits the outer condition and then the leaf, and checks the served CSS each time. On an ASAN build it turns leak checking on for the dev server process. On main that process exits with2016 byte(s) leaked in 51 allocation(s), and the test fails withDevServer panicked.css.test.ts65 pass and 1 skip,dev/css.test.ts16 pass. On a release build: 66 pass and 16 pass. Also on the debug build:test/bundler/css/*.test.ts(25 pass),bundler_html.test.ts(24),test/js/bun/css/css.test.ts(1,314) andcss-loader.test.ts(2).Measured
Main
ea6a2c7b7cand this branch, debug builds with ASAN:(min-width: 100px),supports(display: grid), andlayer(a.b)with no conditional import below it. The last input is theurl()case in Downsides.diff -r).MediaList::deep_clonecalls in one build (gdb breakpoint hit counts): one conditional import 2 to 0, a nested pair 6 to 0, an unconditional import 0 to 0.__rust_alloccalls in one build of 100 imports: no condition 14,708 to 14,708,(min-width: Npx)14,808 to 14,808,env(a, Npx)15,208 to 15,008,calc(Npx + 1em)16,990 to 15,802.<CssImportOrder as Drop>::dropcalls in the build with no condition: 101 to 0. The symbol is gone.The earlier base
fd539ca7c0, with and without this diff. The files of the diff are equal on both bases, apart from one unrelated hunk incss_parser.rs..text: 81,314,645 to 81,312,085 bytes, 2,560 bytes smaller (llvm-size)..rodata,.dataand.bssare equal.bloatyis not installed.size_of::<CssImportOrder>(): 104 to 96 bytes. One element of a condition list: 104 bytes (ImportConditions) to 8 bytes (BackRef).Bun.buildcalls with a 2,000-tokenenv()fallback: main 173 to 176 MB, this diff 4 to 6 MB. A control with no condition grows 2 to 9 MB.bun buildof 2,000 unconditional imports, 12 interleaved pairs: main median 127,524,727 (127,141,172 to 128,844,651), this diff median 127,576,112 (127,355,329 to 128,146,248). The medians differ by 0.04%, inside the spread of each build.perfis not installed. A small C program read the counter withperf_event_open.css.test.ts65 pass and 1 skip,dev/css.test.ts16 pass.cargo check --workspacepassed for 9 of the 12 CI targets: linux gnu and musl, android, darwin, and x64 windows. The other 3 did not run.Self-review
The review raised 22 concerns. 20 are addressed in the diff, the tests or this text. Two are addressed in part:
@importin the dev server test. The page of that test loads in a client, and an external import would make the client contact the network. The spawned builds cover external imports.Open PRs that touch the same lines
Conflict regions in the files of this PR when each head is merged with this branch (
git merge-tree), next to the count for main alone:css/URLInEnvFallbackOfAtImportMediaQueryon ASAN builds because of this leak. With both changes applied, that test passed on an ASAN build with the skip removed.