fix(v8): allow non-ErrorInstance objects in Error.prepareStackTrace#27712
fix(v8): allow non-ErrorInstance objects in Error.prepareStackTrace#27712hasesho05 wants to merge 3 commits intooven-sh:mainfrom
Conversation
Error.prepareStackTrace's default implementation (jsFunctionDefaultErrorPrepareStackTrace) required its first argument to be a JSC ErrorInstance via jsDynamicCast<ErrorInstance*>. This threw TypeError for ES5-style Error subclasses that use Object.create(Error.prototype) without calling super()/Error.call(this). This caused a critical hang when: 1. @babel/core replaces Error.prepareStackTrace with a "stackTraceRewriter" that chains to the original default prepareStackTrace 2. @xmldom/xmldom's ES5-style ParseError calls Error.captureStackTrace(this) 3. The chained call to the default prepareStackTrace throws TypeError 4. xmldom's SAX parser enters error recovery, leading to an infinite loop in the position() function's linePattern regex (/\r\n?|\n|$/g) The fix relaxes the check from jsDynamicCast<ErrorInstance*> to isObject()+getObject(), matching V8's behavior which accepts any object as the first argument to Error.prepareStackTrace. Fixes oven-sh#27708 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughRelaxed first-argument validation in the default JS stack-trace formatter to accept generic objects, added regression tests for Error.captureStackTrace behavior with ES5-style subclasses and plain objects when prepareStackTrace is overridden, and added a multi-file regression suite (with a new SVG validation helper) reproducing the xmldom+svgr hang. 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. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/regression/issue/027708.test.ts`:
- Around line 170-193: The test can hang because stdout/stderr are piped and
never consumed and the child process isn't scoped with resource cleanup; change
the spawn usage to use resource-scoped syntax (await using const proc =
Bun.spawn(...)) and ensure both proc.stdout and proc.stderr are actively drained
(e.g., pipe them to a no-op/writable sink or consume via read loop) so pipe
backpressure cannot block the child; replace the raw setTimeout-based Promise
with Bun.sleep or an awaited race like Promise.race([proc.exited,
Bun.sleep(timeout)]) to avoid unscoped timers, and keep the existing kill+await
proc.exited logic so cleanup runs under the await using scope.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/bun.js/bindings/FormatStackTraceForJS.cpptest/js/node/v8/capture-stack-trace.test.jstest/regression/issue/027708.test.ts
test/regression/issue/027708.test.ts
Outdated
| const proc = Bun.spawn({ | ||
| cmd: [bunExe(), "test"], | ||
| cwd: dir, | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| stdout: "pipe", | ||
| }); | ||
|
|
||
| const timeout = 30_000; | ||
| const raceResult = await Promise.race([ | ||
| proc.exited, | ||
| new Promise<"timeout">((resolve) => setTimeout(() => resolve("timeout"), timeout)), | ||
| ]); | ||
|
|
||
| if (raceResult === "timeout") { | ||
| proc.kill(); | ||
| await proc.exited; | ||
| throw new Error( | ||
| `bun test hung for ${timeout / 1000}s — prepareStackTrace + ES5 Error subclass issue not fixed (issue #27708)`, | ||
| ); | ||
| } | ||
|
|
||
| // Verify tests actually passed | ||
| expect(proc.exitCode).toBe(0); |
There was a problem hiding this comment.
Process lifecycle here can self-induce hangs (undrained pipes) and violates test harness rules (setTimeout, missing await using).
stdout/stderr are piped but never consumed, which can block the child on pipe backpressure and mimic a hang. Also, this test uses setTimeout and does not scope Bun.spawn with await using.
🔧 Suggested adjustment
- const proc = Bun.spawn({
+ await using proc = Bun.spawn({
cmd: [bunExe(), "test"],
cwd: dir,
env: bunEnv,
- stderr: "pipe",
- stdout: "pipe",
+ stderr: "ignore",
+ stdout: "ignore",
});
- const timeout = 30_000;
- const raceResult = await Promise.race([
- proc.exited,
- new Promise<"timeout">((resolve) => setTimeout(() => resolve("timeout"), timeout)),
- ]);
-
- if (raceResult === "timeout") {
- proc.kill();
- await proc.exited;
- throw new Error(
- `bun test hung for ${timeout / 1000}s — prepareStackTrace + ES5 Error subclass issue not fixed (issue `#27708`)`,
- );
- }
-
- // Verify tests actually passed
- expect(proc.exitCode).toBe(0);
+ // Non-hang is enforced by this test's timeout.
+ const exitCode = await proc.exited;
+ expect(exitCode).toBe(0);As per coding guidelines: "Use await using or using to ensure proper resource cleanup for APIs like Bun.spawn" and "Do not use setTimeout in tests; instead await the condition to be met."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/027708.test.ts` around lines 170 - 193, The test can
hang because stdout/stderr are piped and never consumed and the child process
isn't scoped with resource cleanup; change the spawn usage to use
resource-scoped syntax (await using const proc = Bun.spawn(...)) and ensure both
proc.stdout and proc.stderr are actively drained (e.g., pipe them to a
no-op/writable sink or consume via read loop) so pipe backpressure cannot block
the child; replace the raw setTimeout-based Promise with Bun.sleep or an awaited
race like Promise.race([proc.exited, Bun.sleep(timeout)]) to avoid unscoped
timers, and keep the existing kill+await proc.exited logic so cleanup runs under
the await using scope.
Address review feedback: - Use `await using` for proper resource cleanup via Symbol.asyncDispose - Set stdout/stderr to "ignore" to prevent pipe backpressure hangs - Remove manual setTimeout-based timeout in favor of test's 60s timeout Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@test/regression/issue/027708.test.ts`:
- Line 65: The regex in the generated sanitizer (HELPERS_TS) is over-escaped in
the template literal: occurrences like "\\\\s" and "\\\\[" produce incorrect
literal escapes in the emitted source; update the template so those sequences
are one level less escaped (use "\\s" and "\\[" in the template) so the final
generated regex source contains the proper \s and \[ escapes used by the RegExp,
e.g. adjust the template literal that produces the pattern used in the sanitized
assignment (the code that generates the /<!DOCTYPE.../ and /<!ENTITY.../
patterns) to remove one layer of backslashes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/regression/issue/027708.test.ts
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 `@test/regression/issue/027708.test.ts`:
- Around line 142-145: The package.json fixture currently uses caret ranges for
dependencies ("@svgr/core", "@svgr/plugin-jsx", "@xmldom/xmldom"); update those
entries to pin exact versions by removing the leading '^' so the values become
"8.1.0" and "0.9.8" respectively, ensuring the generated fixture uses
deterministic, exact dependency versions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/regression/issue/027708.test.ts
| "@svgr/core": "^8.1.0", | ||
| "@svgr/plugin-jsx": "^8.1.0", | ||
| "@xmldom/xmldom": "^0.9.8", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/regression/issue/027708.test.ts | sed -n '135,155p'Repository: oven-sh/bun
Length of output: 852
🏁 Script executed:
# Check the complete devDependencies section to understand the context
rg -A 20 "devDependencies:" test/regression/issue/027708.test.tsRepository: oven-sh/bun
Length of output: 622
🏁 Script executed:
# Check if there are other test fixtures and how they specify dependencies
fd "\.test\.ts" test/regression/issue/ -x basename {} | head -5Repository: oven-sh/bun
Length of output: 146
🏁 Script executed:
# Look for patterns in other test files with package.json generation
rg -l "devDependencies" test/regression/ --type tsRepository: oven-sh/bun
Length of output: 260
🏁 Script executed:
# Check how other regression tests specify dependencies
rg -B 3 -A 5 "devDependencies" test/regression/issue/11806.test.ts test/regression/issue/07740.test.tsRepository: oven-sh/bun
Length of output: 1158
🏁 Script executed:
# Look at the issue this regression test is for - what problem was it solving?
# Check if there are comments in the test explaining the purpose
rg -B 5 "27708\|hang\|xmldom\|svgr" test/regression/issue/027708.test.ts | head -50Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check git history or comments about why caret ranges might be used
git log --oneline -n 20 -- test/regression/issue/027708.test.ts 2>/dev/null || echo "No git history available"Repository: oven-sh/bun
Length of output: 133
🏁 Script executed:
# Check if there are any guidelines about version pinning in test fixtures
rg "version|pin|exact|caret" test/CLAUDE.md CLAUDE.md test/regression/README.md 2>/dev/null | head -20Repository: oven-sh/bun
Length of output: 120
🏁 Script executed:
# Get more context about the test and the issue it addresses
rg -B 10 "bun test should not hang with 7\+" test/regression/issue/027708.test.tsRepository: oven-sh/bun
Length of output: 448
🏁 Script executed:
# Check what the test is actually verifying - does it check for a hang/timeout?
rg -A 30 "bun test should not hang" test/regression/issue/027708.test.ts | head -50Repository: oven-sh/bun
Length of output: 964
🏁 Script executed:
# Look for the GitHub issue number reference or comments about why these specific versions
rg "27708\|issue" test/regression/issue/027708.test.ts | head -20Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if other tests that use exact versions had a specific reason (like reproducing a bug)
rg -B 5 -A 5 "1\.0\.0" test/regression/issue/07740.test.tsRepository: oven-sh/bun
Length of output: 465
🏁 Script executed:
# Check what the specific issue `#27708` was about - search for any related commits or PRs
git log --all --oneline --grep="27708\|hang.*svgr\|hang.*xmldom" 2>/dev/null | head -10 || echo "No git log available"Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Examine the complete test to see if it validates against the behavior at these specific versions
rg -A 80 "bun test should not hang with 7\+" test/regression/issue/027708.test.ts | tail -60Repository: oven-sh/bun
Length of output: 1327
🏁 Script executed:
# Check if there are any version constraints or lock files in the test fixture approach
rg "bun\.lockb|yarn\.lock|package-lock\.json" test/regression/issue/027708.test.tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check the purpose: does the test verify behavior at THESE specific versions, or general behavior?
rg -B 2 -A 10 "timeout\|hang\|expect" test/regression/issue/027708.test.ts | grep -A 5 "const result"Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Verify the complete test flow and check if there are assertions about specific package behavior
rg -A 120 "bun test should not hang with 7\+" test/regression/issue/027708.test.ts | tail -50Repository: oven-sh/bun
Length of output: 1327
🏁 Script executed:
# Search for any documentation about regression test practices regarding dependency versions
find test -name "README*" -o -name "CLAUDE*" | xargs grep -l "regression\|version" 2>/dev/null | head -5Repository: oven-sh/bun
Length of output: 221
🏁 Script executed:
# Check if the versions in the caret ranges have known breaking changes
web_search
npm registry `@svgr/core` versions after 8.1.0Repository: oven-sh/bun
Length of output: 206
🌐 Web query:
@svgr/core 8.1.0 package versions npm
💡 Result:
- Package:
@svgr/core(npm) - Latest (as of March 3, 2026):
8.1.0(npmjs.com) - Published date for
8.1.0: Aug 15, 2023 (security.snyk.io) - Recent versions (newest → older):
8.1.0,8.0.0,7.0.0,6.5.1,6.5.0,6.4.0,6.3.1, … (security.snyk.io)
To list all versions from npm locally:
npm view `@svgr/core` versions --jsonCitations:
- 1: https://www.npmjs.com/package/%40svgr/core?utm_source=openai
- 2: https://security.snyk.io/package/npm/%40svgr%2Fcore/versions?utm_source=openai
- 3: https://security.snyk.io/package/npm/%40svgr%2Fcore/versions?utm_source=openai
🌐 Web query:
@xmldom/xmldom 0.9.8 package breaking changes
💡 Result:
Are there breaking changes in 0.9.8 itself?
No. The 0.9.8 changelog entry lists fixes/performance/chore changes, but no “BREAKING CHANGE” notes for that release. [1]
Breaking changes you’re likely hitting when upgrading to 0.9.8 (from 0.8.x / earlier 0.9.x)
From the project changelog, the notable breaking changes in the 0.9.x line (that still apply when you land on 0.9.8) include:
DOMParser.parseFromStringnow requiresmimeTypeas the 2nd argument. [2]- HTML parsing rules now require the passed
mimeTypeto match'text/html'(not just namespace-driven behavior). [3] - Type change:
DOMParser.parseFromStringreturn type corrected toDocument | undefined(you may need to handleundefinedin TS). [3] DOMParser.optionsinstance property removed; use the individual readonly option properties instead; alsolocatoroption is now just a boolean. [3]- Removed non-spec boolean toggle for “HTML rules” on:
XMLSerializer.serializeToStringNode.toStringDocument.toString[4]
- Attribute/NamedNodeMap behavior aligned with DOM spec:
- iteration over attributes now in the “right” order
- non-existing attributes return
nullinstead ofundefined - similar
nullbehavior forAttr.namespaceURIandAttr.prefix[5]
If you tell me what version you’re upgrading from (e.g., 0.8.11 vs 0.9.7) and what breaks (error message / failing test), I can narrow it to the exact change.
🏁 Script executed:
# Check test/CLAUDE.md for any guidance on regression test practices
cat test/CLAUDE.md | grep -A 10 -B 5 "regression\|version"Repository: oven-sh/bun
Length of output: 780
🏁 Script executed:
# Check if there are similar test patterns with dynamic vs pinned versions
rg "devDependencies.*\^|devDependencies.*[0-9]+" test/regression/issue/*.test.ts -A 3 | head -40Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Verify the exact claim: check if there are any newer versions of these packages currently
npm view `@svgr/core`@"^8.1.0" version 2>/dev/null || echo "Checking npm would resolve to:"Repository: oven-sh/bun
Length of output: 60
🏁 Script executed:
# Check if other test fixtures in the repo use exact vs caret versions consistently
rg "devDependencies.*{" test/regression/issue/*.test.ts -A 10 | grep -E "^\s+\"[^\"]+\":\s+[\"\^]" | sort | uniq -cRepository: oven-sh/bun
Length of output: 154
🏁 Script executed:
# Check version patterns in regression test fixtures
rg "devDependencies" test/regression/issue/*.test.ts -A 10 | grep -E '\"[^\"]+\":\s+' | head -30Repository: oven-sh/bun
Length of output: 1348
🏁 Script executed:
# Check what the actual resolved versions would be if someone ran bun install today
python3 <<'PYEOF'
import re
from pathlib import Path
# Read the test file
content = Path("test/regression/issue/027708.test.ts").read_text()
# Find devDependencies block
match = re.search(r"devDependencies:\s*\{([^}]+)\}", content, re.DOTALL)
if match:
deps = match.group(1)
print("Current dependencies in test/regression/issue/027708.test.ts:")
for line in deps.split('\n'):
if line.strip():
print(f" {line.strip()}")
PYEOFRepository: oven-sh/bun
Length of output: 204
🏁 Script executed:
# Check if there's a pattern in the codebase about regression test fixture versions
rg "@svgr|@xmldom" test/regression/ -B 2 -A 2Repository: oven-sh/bun
Length of output: 2500
Use exact versions for the fixture dependencies to ensure regression test determinism.
Lines 142–145 use caret ranges in the generated package.json. While these currently resolve to the latest versions, pinning exact versions (remove the ^ prefix) makes the regression test's conditions explicit and protects against any future patch or minor releases that could affect test behavior. This aligns with best practices for regression test stability.
🔧 Proposed fix
devDependencies: {
- "@svgr/core": "^8.1.0",
- "@svgr/plugin-jsx": "^8.1.0",
- "@xmldom/xmldom": "^0.9.8",
+ "@svgr/core": "8.1.0",
+ "@svgr/plugin-jsx": "8.1.0",
+ "@xmldom/xmldom": "0.9.8",
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/027708.test.ts` around lines 142 - 145, The
package.json fixture currently uses caret ranges for dependencies ("@svgr/core",
"@svgr/plugin-jsx", "@xmldom/xmldom"); update those entries to pin exact
versions by removing the leading '^' so the values become "8.1.0" and "0.9.8"
respectively, ensuring the generated fixture uses deterministic, exact
dependency versions.
1 participant
Summary
Error.prepareStackTraceto accept any object (not just JSCErrorInstance) as its first argument, matching V8 behaviorbun testhangs indefinitely when@babel/core+@xmldom/xmldomare used togetherRoot Cause
jsFunctionDefaultErrorPrepareStackTraceinFormatStackTraceForJS.cppusedjsDynamicCast<ErrorInstance*>to validate its first argument, throwingTypeErrorfor any non-ErrorInstanceobject. This caused a critical hang through this chain:Error.prepareStackTracewith astackTraceRewriterfunction (inrewrite-stack-trace.js) that chains to the original defaultprepareStackTraceParseError) withObject.create(Error.prototype)— nosuper()call, so JSC doesn't recognize it as anErrorInstanceError.captureStackTrace(this, ParseError)is called inside theParseErrorconstructor, bun calls the chainedprepareStackTrace, which calls the default implementationTypeError: First argument must be an Error objectParseErrorfrom being properly thrown during malformed SVG parsingposition()function'slinePatternregex (/\r\n?|\n|$/g)Fix
Changed the check from
jsDynamicCast<ErrorInstance*>toisObject()+getObject(), accepting any JavaScript object. This matches V8's behavior —Error.prepareStackTrace(error, structuredStackTrace)should work with any error-like object, not just native Error instances.Test plan
test/js/node/v8/capture-stack-trace.test.js:Error.captureStackTrace+ customprepareStackTraceError.captureStackTrace+ customprepareStackTracetest/regression/issue/027708.test.ts🤖 Generated with Claude Code