Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 69 additions & 23 deletions test/js/node/vm/happy-dom-vm-16277.test.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,73 @@
import { expect, test } from "bun:test";
import { Window } from "happy-dom";
test("reproduction", async (): Promise<undefined> => {
expect.assertions(1);
for (let i: number = 0; i < 2; ++i) {
// TODO: have a reproduction of this that doesn't depend on a 10 MB file.
const response: Response = new Response(`<!DOCTYPE html>
<html>
<head>
<script
id="base-js"
src="https://www.youtube.com/s/desktop/6ed1dd74/jsbin/desktop_polymer_legacy_browsers.vflset/desktop_polymer_legacy_browsers.js"
nonce="4oKS2biXokC0utrX4MKrsQ"></script>
</head>
</body>
</html>`);
const window: Window = new Window({ url: "http://youtube.com" });
const localStorage = window.localStorage;
global.window = window;
global.document = window.document;
localStorage.clear();
document.body.innerHTML = await response.text();
import vm from "node:vm";

// Regression tests for https://github.com/oven-sh/bun/issues/16277
//
// The original report hit this via happy-dom loading a large external script
// that happened to tail-call a non-function. The root cause is that when a
// strict-mode tail call site invokes a value that is not callable, JSC
// reconstructs the elided tail-call frame from CallLinkInfo with a null
// `callee` and a non-null `codeBlock`. Bun's stack formatter dereferenced
// `frame.callee()` unconditionally, which segfaulted at address 0x5.
//
// The original test shelled out to YouTube via happy-dom's synchronous fetch
// (child_process.execFileSync) to obtain such a script, which made the test
// both network-dependent and very slow. These tests reproduce the exact crash
// deterministically and locally.

function makeTailCallError(): unknown {
"use strict";
function inner(fn: () => void): void {
"use strict";
return fn();
}
function outer(fn: () => void): void {
"use strict";
return inner(fn);
}
try {
outer(null as any);
} catch (e) {
return e;
}
throw new Error("unreachable: outer(null) did not throw");
}

test("error stack includes the reconstructed tail-call frame", () => {
const err = makeTailCallError() as Error;
// The reconstructed frame has codeBlock=inner's but callee=null; the fix is
// that Bun's stack formatter null-checks the callee before `->getObject()`.
const stack = String(err.stack);
expect(stack.includes("is not a function") || stack.includes("not callable")).toBe(true);

Check warning on line 41 in test/js/node/vm/happy-dom-vm-16277.test.ts

View check run for this annotation

Claude / Claude Code Review

Inconsistent error-message assertion between test 1 and test 2

The error-message assertions in test 1 and test 2 disagree: line 41 hedges with `|| stack.includes("not callable")` while line 61 in the vm-context test asserts strictly `toContain("is not a function")`. Your own verification table shows test 2 passes on every configuration (1.1.42/1.1.44/debug+ASAN/`useJIT=0`), which proves the strict form works everywhere — so the `"not callable"` branch on line 41 is dead weight that weakens the assertion. Drop the OR and use `expect(stack).toContain("is not
Comment thread
robobun marked this conversation as resolved.
Outdated
expect(stack).toContain("inner");
// `outer` was the tail caller of `inner`; its own frame is legitimately gone
// (tail-call semantics), but `inner` must be present.
});

test("same path through a node:vm context", () => {
const ctx = vm.createContext({});
const err = vm.runInContext(
`"use strict";
function inner(fn) { "use strict"; return fn(); }
function outer(fn) { "use strict"; return inner(fn); }
var caught;
try { outer(null); } catch (e) { caught = e; }
caught;`,
ctx,
);
expect(err).toBeTruthy();
const stack = String((err as Error).stack);
expect(stack).toContain("inner");
expect(stack).toContain("is not a function");
});

// This test passes by simply not crashing.
expect().pass();
test("error whose stack is materialized lazily during GC does not crash", () => {
// Create Errors with tail-call frames and drop them without touching `.stack`
// so JSC materializes the stack string in the ErrorInstance finalizer, which
// runs under Heap::runEndPhase.
for (let i = 0; i < 64; i++) makeTailCallError();
Bun.gc(true);

Check warning on line 69 in test/js/node/vm/happy-dom-vm-16277.test.ts

View check run for this annotation

Claude / Claude Code Review

GC-finalizer test drops errors so finalizeUnconditionally never runs on them

Test 3's design is inverted: `ErrorInstance::finalizeUnconditionally` runs via `Heap::finalizeMarkedUnconditionalFinalizers`, which iterates `forEachMarkedCell` — it fires only on errors that **survive** the collection, not on ones that are dropped. Since all 64 errors here are unreachable at `Bun.gc(true)`, they're simply swept without materializing `.stack`, so the loop + GC can be deleted without changing the outcome and the test reduces to a repeat of test 1's eager-`.stack` path. To actuall
Comment thread
robobun marked this conversation as resolved.
Outdated
// If we got here the finalizer did not crash; finish with a positive check.
const stack = String((makeTailCallError() as Error).stack);
expect(stack).toContain("inner");

Check warning on line 72 in test/js/node/vm/happy-dom-vm-16277.test.ts

View check run for this annotation

Claude / Claude Code Review

Crash-regression repro runs in-process rather than as a spawned fixture

Since this file guards a segfault, consider running at least test 3 (the pure "does not crash during GC finalizer" check) via `Bun.spawn([bunExe(), "-e", ...])` and asserting `exitCode === 0` / `signalCode === null`. As written, a regression of #16277 segfaults the test-runner process itself at address 0x5, taking every subsequent test in the shard with it rather than reporting one clean failure. A single spawn under debug+ASAN is still ~10× faster than the 16.5s baseline this replaces, so it do
Comment thread
robobun marked this conversation as resolved.
Outdated
});
Loading