Conversation
StructureMemoryManager reserves the Structure heap at startup. When the OS refuses every size, or mimalloc refuses the range that was reserved, a RELEASE_ASSERT aborts the process. In Bun the abort shows as a crash report, but the cause is a limit of the process (ulimit -v, ulimit -d, vm.overcommit_memory=2). The constructor now calls Bun__structureHeapReservationFailed with the sizes it tried, when Bun defines that function. The function does not return. The JSC shell does not define it and keeps the assertions.
![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 findings, I also checked the mappedHeapSize * 2 passed as smallestTriedBytes at StructureAlignedMemoryAllocator.cpp:134 — after the 8-iteration loop exits without a reservation, mappedHeapSize has been halved once past the last size attempted, so the doubling recovers the correct value.
Extended reasoning...
The change adds a weak noreturn Bun hook called at two Structure heap reservation failure points in Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp and lists the symbol in both linker undefined-symbol lists in Source/cmake/WebKitCompilerFlags.cmake; it touches no security-sensitive surface. Two inline findings were already reported, so this note only records the size-arithmetic check that was ruled out.
|
Preview build of 2a0f26a: |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 5 included reviews per hour; 1 remain after this review. WalkthroughBun builds now report structure-heap reservation failures through an optional callback. Apple and non-Apple Bun event-loop linker flags retain the callback symbol. ChangesStructure-heap failure reporting
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No verified defect requires holding this change. The allocator retains its assertion if the failure-reporting hook returns; normal checks should cover the affected link targets. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, fix, affected behavior, testing, and performance impact. It does not follow the repository template because it omits a Bugzilla URL, the required review line, and the template-style changed-file and function list. Resolution Add the associated Bugzilla bug URL, include the required “Reviewed by NOBODY (OOPS!).” line or actual reviewer information, and list each changed file with the relevant functions or classes. Ensure the pull request is also linked from Bugzilla and has the required labels and assignment if applicable.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
The declaration is no longer noreturn. A definition that returns now reaches RELEASE_ASSERT_NOT_REACHED, where before the behavior was undefined.
2 participants
Pairs with oven-sh/bun#39967, which defines the function.
Problem
StructureMemoryManager::StructureMemoryManager()aborts when it cannot reserve the Structure heap.RELEASE_ASSERT(g_jscConfig.startOfStructureHeap, ...)fires when the OS refuses all 8 sizes.RELEASE_ASSERT(mi_manage_os_memory_ex(...))fires when mimalloc refuses the range that was reserved.ulimit -v,ulimit -d,vm.overcommit_memory=2). In Bun the abort prints "Bun has crashed" and sends a crash report (Sentry BUN-4NF7).Fix
Bun__structureHeapReservationFailed(largestTriedBytes, smallestTriedBytes, reservedBytes)at both failures, when the embedder defines it. Bun prints an error with the sizes and the limits of the process, then exits with status 1.jscshell does not define the function, so it keeps both assertions. A definition that returns reachesRELEASE_ASSERT_NOT_REACHED().WebKitCompilerFlags.cmakelists the symbol with the other hooks. The macOS link ofjscneeds the-Uentry for a symbol that stays undefined.bun-webkit-linux-amd64-ltolane of this pull request (2a0f26a). Thejscshell still aborts with status 134 at both assertions. A bun release build with crash_handler: report an exhausted address space during JSC initialization as an error, not a crash bun#39967 prints the error and exits 1 there, at each of 16ulimit -vand 14ulimit -dlimits where the base aborts.Background
Structure. AStructureIDis a 32-bit offset into it, so JSC reserves the range in one piece at startup. The constructor asks for 4 GB and halves the size up to 7 times.RunLoopBun.cppexplains why there is no fallback definition.SIGABRTand has to guess the cause.RLIMIT_ASand replaces the mimalloc assertion with a fallback. The first assertion stays reachable after [JSC] Fit the JIT pool and the Structure heap reservations to RLIMIT_AS #586, for example underulimit -d. If [JSC] Fit the JIT pool and the Structure heap reservations to RLIMIT_AS #586 merges first, the second call goes away in the rebase.Downsides
ulimit -v 400000(gdb step count, 3 of 3 runs identical).