test(vm): replace network-dependent happy-dom #16277 repro with deterministic tail-call repro - #36823
Conversation
…ministic tail-call repro The previous test loaded a 10MB script from youtube.com via happy-dom's synchronous fetch (ChildProcess.execFileSync), which made it both slow (~11s on ASAN lanes) and network-dependent. In sandboxed environments where youtube.com is unreachable the fetch fails early and the test passes even on the broken Bun 1.1.43, so it was not actually guarding the regression. The root cause of #16277 is that a strict-mode tail call to a value that is not callable produces an error whose stack contains a frame that JSC reconstructs from CallLinkInfo with callee=null and codeBlock!=null. Bun's stack formatter dereferenced frame.callee() unconditionally. The new tests reproduce that exact state directly (both in the main realm and inside a node:vm context) and assert on the resulting stack. They segfault Bun 1.1.43 at address 0x5 and pass on 1.1.42/1.1.44+. No network, no subprocess, no happy-dom import; file runs in ~2.4s under debug+ASAN vs ~16s before.
WalkthroughThe test replaces the network-dependent reproduction with deterministic local tests for direct execution, ChangesVM stack regressions
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…e crash in subprocess - drop '|| not callable' hedge so test 1 and test 2 agree - test 3 now retains the errors across Bun.gc(true) and creates the tail-call functions per-iteration via new Function, so the captured frame's CodeBlock is unmarked and ErrorInstance::finalizeUnconditionally actually calls computeErrorInfo (the Heap::runEndPhase path in the original crash trace) - test 3 runs in a spawned child so a regression reports as a clean failure instead of segfaulting the test runner
There was a problem hiding this comment.
LGTM — all three prior review notes are addressed in 0f0849f.
What was reviewed:
- Test 3 now retains errors and drops the per-iteration
new FunctionCodeBlocks, sofinalizeUnconditionallyis actually reached; author verified the 1.1.43 child segfaults insideBun.gc(true). - Crash repro is isolated in a spawned child with
stderr/stdout/signalCode/exitCodeassertions and concurrent pipe draining. - Message assertions are now consistent (
toContain("is not a function")in both tests); no external network, usesbunEnv/bunExe.
Extended reasoning...
Overview
Test-only change to test/js/node/vm/happy-dom-vm-16277.test.ts. Replaces a network-dependent happy-dom repro (which spawned child processes to fetch a 10 MB YouTube script) with three deterministic local tests that reproduce the #16277 null-callee tail-call frame directly: eager .stack read, the same through node:vm, and the ErrorInstance::finalizeUnconditionally GC-finalizer path in a spawned subprocess.
Prior review follow-up
I left three nits on the first revision (5a0ec66): test 3's finalizer semantics were inverted (dropped errors don't reach finalizeUnconditionally), the crash repro ran in-process, and test 1's message assertion was OR-weakened. All three were addressed in 0f0849f exactly as suggested — errors are now retained across GC while their per-iteration new Function CodeBlocks become unreachable, test 3 runs under Bun.spawn([bunExe(), "-e", ...]) with signalCode === null / exitCode === 0 assertions, and both message checks use the strict form. All three threads are marked resolved. The bug hunting system found nothing on this revision.
Security risks
None. Test-only; removes an external-network dependency rather than adding one.
Level of scrutiny
Low-medium. No production code touched. The main risk with a repro rewrite is losing the coverage the old test provided; the PR description demonstrates the new tests segfault on 1.1.43 (the regression version) and pass on current debug+ASAN, release, and useJIT=0, and the author re-verified the finalizer-path crash location after the fix commit. The subprocess test follows harness conventions (bunEnv, bunExe, await using, concurrent Promise.all pipe drain, stderr asserted before exitCode).
Other factors
The old test violated the "never contact the public internet" rule and was not actually guarding the regression on sandboxed runners (passed on 1.1.43 when DNS failed). The replacement is hermetic, ~4× faster under debug+ASAN, and has 9 real assertions vs. the previous expect().pass(). happy-dom integration coverage remains in test/integration/svelte/ and test/cli/create/create-jsx.test.ts. Straightforward improvement.
…coverage Self-review pointed out that the other happy-dom references in the suite either run under Node.js (bake/client-fixture.mjs), are commented out (svelte/server-side.test.ts), or use GlobalWindow which overrides setupVMContext to a no-op (create-jsx.test.ts). So this file was the only place vm.createContext ran on a happy-dom Window in Bun. Add that back without the external fetch: disableJavaScriptFileLoading, set innerHTML, assert vm.isContext(window) and the parsed DOM.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/vm/happy-dom-vm-16277.test.ts`:
- Line 111: Wrap the test body using the window from the happy-dom setup in a
try/finally, registering cleanup before any assertions and calling
window.happyDOM.close() in the finally block. Preserve the existing assertions
and ensure cleanup runs when setup or an assertion throws.
- Line 108: Add a known storage entry before localStorage.clear() in the test,
then assert via getItem() that the entry returns null afterward, ensuring the
clear operation is tested on populated storage.
🪄 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: ffb5c914-c83d-407c-9efd-6c10285b2897
📒 Files selected for processing (1)
test/js/node/vm/happy-dom-vm-16277.test.ts
…localStorage.clear()
There was a problem hiding this comment.
LGTM — all prior feedback addressed; deterministic, hermetic replacement for a network-dependent test.
What was reviewed
- Test 3 now retains errors and drops per-iteration
new FunctioncodeBlocks sofinalizeUnconditionallyactually fires, and runs in a spawned child withsignalCode === null/exitCode === 0assertions. - Tests 1/2 now assert
"is not a function"strictly (hedge dropped); pipe draining usesPromise.all; happy-dom Window is closed infinally. - No network access remains:
disableJavaScriptFileLoadingset and the injected HTML has no<script>tag;localStorage.clear()was dropped as non-load-bearing.
Extended reasoning...
Overview
Test-only change to test/js/node/vm/happy-dom-vm-16277.test.ts. Replaces a single expect().pass() test that fetched a 10 MB YouTube script via happy-dom's synchronous execFileSync loader with four deterministic tests: (1) eager .stack on a strict-mode tail-call-to-null error, (2) same via vm.runInContext, (3) a spawned child that retains 64 such errors while their per-iteration new Function codeBlocks become unreachable, then Bun.gc(true) to drive ErrorInstance::finalizeUnconditionally, asserting stderr/stdout/signalCode/exitCode, and (4) a network-free happy-dom Window smoke test for vm.createContext coverage with try/finally cleanup.
Security risks
None. Test-only; no runtime code touched. The change removes an external network fetch (youtube.com), which was itself a REVIEW.md violation ("Never contact external network hosts").
Level of scrutiny
Low-to-moderate. The old test was effectively vacuous in sandboxed CI (passed on the broken 1.1.43 build when the fetch failed) and had a standing TODO to replace it. The new tests are strictly better on every REVIEW.md axis: hermetic, real assertions instead of expect().pass(), crash repro isolated in a subprocess, and verified to segfault on 1.1.43 inside Bun.gc(true) (the finalizer path) per the author's follow-up. The subtle JSC-internals reasoning (marked-cell finalizer semantics) was the one thing worth scrutinizing — I raised it in the first review round, the author fixed it in 0f0849f and confirmed empirically against 1.1.43.
Other factors
All five prior review comments (three from me, two from coderabbit) are resolved with corresponding commits (0f0849f, a930a25). Harness conventions are followed: bunEnv/bunExe, concurrent pipe draining via Promise.all, stderr/stdout asserted before exitCode, await using on the spawned process, try/finally around the happy-dom Window. Tests 1 and 2 still run the crash repro in-process, which I previously noted as a nit rather than a blocker — the primary crash guard (test 3) is now subprocess-isolated, and the in-process tests make positive .stack-content assertions that are more naturally expressed inline. The gate evidence shows 4 pass / 11 expect() on debug+ASAN.
…h deterministic tail-call repro (oven-sh#36823) ## What `test/js/node/vm/happy-dom-vm-16277.test.ts` was taking ~11s on the `debian 13 x64-asan` lane (build 88024) and had a `TODO: have a reproduction of this that doesn't depend on a 10 MB file`. This PR resolves that TODO. ## Why the old test was slow and unreliable The old test set `document.body.innerHTML` to HTML containing `<script src="https://www.youtube.com/.../desktop_polymer_legacy_browsers.js">`. happy-dom's synchronous script loader then spawned `ChildProcess.execFileSync(process.argv[0], ['-e', <http-request>])` twice to fetch that 10 MB bundle from YouTube and evaluated it in the vm context. Under a debug+ASAN build each child process spawn costs several seconds regardless of outcome. It also meant coverage depended on external network reachability: in sandboxed runners where `youtube.com` does not resolve, the fetch throws early and the test **passes on Bun 1.1.43** (the broken version), so it was not guarding the regression there. ## Root cause of oven-sh#16277 When a strict-mode tail call site invokes a value that is not callable, JSC reconstructs the elided caller frame from `CallLinkInfo` via `Interpreter::getStackTrace`, emitting a `StackFrame` with `codeBlock` set and `callee == nullptr` (the `StackFrame(VM&, owner, CodeBlock*, BytecodeIndex)` constructor). Bun's `formatStackTrace` dereferenced `frame.callee()->getObject()` without a null check, which segfaulted at address `0x5`. PR oven-sh#16280 added the null check. ## New tests 1. Strict-mode `return fn()` where `fn` is `null`, then assert the resulting `.stack` contains the reconstructed `inner` frame and `"is not a function"`. 2. The same pattern run inside a `node:vm` context. 3. In a spawned child: create such errors repeatedly via per-iteration `new Function` and retain them, then `Bun.gc(true)`, so the captured frame's `codeBlock` is unmarked and `ErrorInstance::finalizeUnconditionally` materializes the stack under `Heap::runEndPhase` (the path in the original crash trace). Asserts `stderr === ""`, `stdout === "64 string\n"`, `signalCode === null`, `exitCode === 0`. 4. A network-free happy-dom smoke test (`new Window()` with `disableJavaScriptFileLoading`, set `innerHTML`, assert `vm.isContext(window)` and the parsed DOM) so `vm.createContext` on a happy-dom `Window` stays covered in Bun. The other happy-dom references in the suite either run under Node.js, are commented out, or use `GlobalWindow` which skips the vm context. ## Verification | Bun version | Result | | --- | --- | | **1.1.43 (regression)** | **`panic(main thread): Segmentation fault at address 0x5`** in test 1; the child in test 3 segfaults inside `Bun.gc(true)` | | current debug+ASAN | 4 pass | | current debug+ASAN, `BUN_JSC_useJIT=0` | 4 pass | | current release | 4 pass | Local timing (`bun bd test test/js/node/vm/happy-dom-vm-16277.test.ts`, debug+ASAN): | | wall-clock | | --- | --- | | before | 16.5s (test body 8.5s; timed out at the default 5s limit without `--timeout`) | | after | 9.5s (happy-dom import dominates under debug+ASAN; 0.3s on release) | Assertions: 1 × `expect().pass()` before → 11 real `expect()` calls now. No external network access. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 0 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/node/vm/happy-dom-vm-16277.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "test/js/node/vm/happy-dom-vm-16277.test.ts" bun test v1.4.0 (a930a25) test/js/node/vm/happy-dom-vm-16277.test.ts: (pass) error stack includes the reconstructed tail-call frame [16.92ms] (pass) same path through a node:vm context [27.00ms] (pass) stack materialized in ErrorInstance::finalizeUnconditionally does not crash [1223.50ms] (pass) happy-dom Window (vm.createContext on the Window object) works [583.74ms] 4 pass 0 fail 11 expect() calls Ran 4 tests across 1 file. [9.23s] Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/node/vm/happy-dom-vm-16277.test.ts | 131 ++++++++++++++++++++++++----- 1 file changed, 109 insertions(+), 22 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/node/vm/happy-dom-vm-16277.test.ts 2 5 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
What
test/js/node/vm/happy-dom-vm-16277.test.tswas taking ~11s on thedebian 13 x64-asanlane (build 88024) and had aTODO: have a reproduction of this that doesn't depend on a 10 MB file. This PR resolves that TODO.Why the old test was slow and unreliable
The old test set
document.body.innerHTMLto HTML containing<script src="https://www.youtube.com/.../desktop_polymer_legacy_browsers.js">. happy-dom's synchronous script loader then spawnedChildProcess.execFileSync(process.argv[0], ['-e', <http-request>])twice to fetch that 10 MB bundle from YouTube and evaluated it in the vm context. Under a debug+ASAN build each child process spawn costs several seconds regardless of outcome.It also meant coverage depended on external network reachability: in sandboxed runners where
youtube.comdoes not resolve, the fetch throws early and the test passes on Bun 1.1.43 (the broken version), so it was not guarding the regression there.Root cause of #16277
When a strict-mode tail call site invokes a value that is not callable, JSC reconstructs the elided caller frame from
CallLinkInfoviaInterpreter::getStackTrace, emitting aStackFramewithcodeBlockset andcallee == nullptr(theStackFrame(VM&, owner, CodeBlock*, BytecodeIndex)constructor). Bun'sformatStackTracedereferencedframe.callee()->getObject()without a null check, which segfaulted at address0x5. PR #16280 added the null check.New tests
return fn()wherefnisnull, then assert the resulting.stackcontains the reconstructedinnerframe and"is not a function".node:vmcontext.new Functionand retain them, thenBun.gc(true), so the captured frame'scodeBlockis unmarked andErrorInstance::finalizeUnconditionallymaterializes the stack underHeap::runEndPhase(the path in the original crash trace). Assertsstderr === "",stdout === "64 string\n",signalCode === null,exitCode === 0.new Window()withdisableJavaScriptFileLoading, setinnerHTML, assertvm.isContext(window)and the parsed DOM) sovm.createContexton a happy-domWindowstays covered in Bun. The other happy-dom references in the suite either run under Node.js, are commented out, or useGlobalWindowwhich skips the vm context.Verification
panic(main thread): Segmentation fault at address 0x5in test 1; the child in test 3 segfaults insideBun.gc(true)BUN_JSC_useJIT=0Local timing (
bun bd test test/js/node/vm/happy-dom-vm-16277.test.ts, debug+ASAN):--timeout)Assertions: 1 ×
expect().pass()before → 11 realexpect()calls now. No external network access.[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file