bun:jsc: propagate JSONParse exceptions from samplingProfilerStackTraces - #37065
bun:jsc: propagate JSONParse exceptions from samplingProfilerStackTraces#37065robobun wants to merge 2 commits into
Conversation
samplingProfilerStackTraces asserted no exception after JSON-parsing the profiler's stack traces, so an out-of-memory error thrown by JSONParse crashed the process with a release assert instead of reaching the caller. Assert before the parse, where nothing can have thrown, and release the throw scope on return, matching generateHeapSnapshotForDebugging.
WalkthroughThe stack-trace function now returns parsed JSON directly through ChangesSampling profiler stack traces
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — two-line exception-scope fix mirroring the same pattern already applied to functionGenerateHeapSnapshotForDebugging in this file.
What was reviewed:
- Verified
stackTracesAsJSON()->toJSONString()is a WTF JSON op that cannot set a JS exception, soreleaseAssertNoException()before the parse is sound. - Checked the redundant
scope.release()fromRELEASE_AND_RETURNafterreleaseAssertNoException()is harmless. - Test follows harness conventions (subprocess, concurrent pipe drain,
{...bunEnv}spread, combined-object assertion); PR description is upfront that it pins the happy path rather than reproducing the OOM.
Extended reasoning...
Overview
Two-line change in src/jsc/modules/BunJSCModule.h's functionSamplingProfilerStackTraces: moves scope.releaseAssertNoException() from after JSONParse to before it, and wraps the return in RELEASE_AND_RETURN so a JSONParse exception (realistically OOM on a large profile) propagates instead of tripping a release assert. Adds a subprocess test to bun-jsc.test.ts exercising startSamplingProfiler() → samplingProfilerStackTraces() under BUN_JSC_validateExceptionChecks=1.
Security risks
None. This is an internal debug/profiling API in bun:jsc; the change only affects how a JSC exception thrown by JSONParse is surfaced (propagate vs. crash).
Level of scrutiny
Low. The fix is mechanical and mirrors functionGenerateHeapSnapshotForDebugging a few lines below in the same file (fixed in #36857 per the description). stackTracesAsJSON() returns a WTF JSON::Value and toJSONString() is pure WTF string building — neither touches the JSC throw scope — so asserting no exception at that point is correct. RELEASE_AND_RETURN after releaseAssertNoException() releases the scope twice, which is idempotent and harmless; the sibling function omits the extra release but both shapes are valid.
Other factors
The PR description is candid that the test passes before and after (OOM inside JSONParse cannot be staged deterministically, and the old releaseAssertNoException() after the parse also satisfies the exception-check validator). The test still adds value by pinning the previously-uncovered happy path with a positive stdout/exitCode assertion, and it follows the file's existing subprocess test conventions (Promise.all on stdout/stderr/exited, combined-object toEqual). No prior reviews or outstanding comments on the timeline.
There was a problem hiding this comment.
LGTM — two-line reorder matching the already-landed #36857 shape, with a subprocess test that pins both branches of the function. The inline nit about bindings.cpp:3933 is a valid follow-up but pre-existing and non-blocking.
Extended reasoning...
Overview
This PR reorders two lines in functionSamplingProfilerStackTraces (src/jsc/modules/BunJSCModule.h): the scope.releaseAssertNoException() moves to before JSONParse, and the return goes through RELEASE_AND_RETURN so that a JSONParse exception (realistically OOM on a large profile) propagates to the caller instead of tripping a release assert. This is byte-for-byte the same fix shape as #36857 applied to functionGenerateHeapSnapshotForDebugging in the same file. A subprocess test in test/js/bun/jsc/bun-jsc.test.ts covers both the never-started throw branch and the parsed-traces success path under BUN_JSC_validateExceptionChecks=1.
Security risks
None. This is an internal debugging/profiling API (bun:jsc), and the change only alters exception-scope bookkeeping around a JSON parse of profiler-generated data.
Level of scrutiny
Low-to-medium. The C++ change is a mechanical two-line reorder in a debug/profiling helper, mirroring an already-merged sibling fix. I confirmed releaseAssertNoException() followed by RELEASE_AND_RETURN is safe (both set the released flag; idempotent), and that nothing between scope declaration and the assert can throw on the success path (the earlier throwException branch returns early; stackTracesAsJSON()->toJSONString() is a WTF operation). The test follows harness conventions: {...bunEnv, ...}, concurrent pipe drain, combined-object assertion, and avoids flaky trace-count assertions by only checking typeof and Array.isArray.
Other factors
The bug hunter flagged one remaining sibling of this pattern at src/jsc/bindings/bindings.cpp:3933-3934 (JSC__JSGlobalObject__generateHeapSnapshot, reachable via console.takeHeapSnapshot()) — I verified it exists. It's a fair whole-class nit per REVIEW.md, but it's pre-existing code this PR doesn't touch and merging without it introduces no regression. The PR description is honest that the test cannot fail-before (OOM inside JSONParse is not deterministically stageable), and the test still adds first-ever coverage of this function's two exits.
|
CI status: 195 of 196 jobs passed on f059495. The one failing lane (debian 13 x64-asan) fails in test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts, which this PR does not touch; the failure is pre-existing on main and has been reported separately. The diff itself is green everywhere, including the other ASAN lanes. |
|
Updated 1:23 PM PT - Aug 6th, 2026
❌ @robobun, your commit f059495 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 37065That installs a local version of the PR into your bun-37065 --bun |
1 participant
Problem
functionSamplingProfilerStackTraces(bun:jsc'ssamplingProfilerStackTraces) insrc/jsc/modules/BunJSCModule.hJSON-parses the profiler's stack traces and then asserts that no exception is pending:JSONParsecan throw, realistically an out-of-memory error while building the parse tree for a long-running profile's stack-trace JSON. When that happens,scope.releaseAssertNoException()crashes the process with aRELEASE_ASSERTinThrowScope::releaseAssertNoException(viafunctionSamplingProfilerStackTraces) instead of propagating the error to the caller, which could otherwise catch it.Fix
Move the no-exception assert to before the parse, where nothing can have thrown (the only earlier throw path returns early, and
stackTracesAsJSON()->toJSONString()is a WTF JSON operation that does not touch the throw scope), and release the scope on return so aJSONParseexception propagates:This is the same shape as #36857, which fixes the identical pattern in
functionGenerateHeapSnapshotForDebuggingin the same file.Test
Adds a test to
test/js/bun/jsc/bun-jsc.test.tscovering both exits of the function: callingsamplingProfilerStackTraces()before the profiler is started throws "Sampling profiler was never started" (this branch previously had no coverage and goes through the same throw scope), and afterstartSamplingProfiler()plus some work it returns the parsed object. The subprocess runs underBUN_JSC_validateExceptionChecks=1and the test asserts the exception-check validator reports no unchecked scopes (the option only has effect on debug builds).A failing-before test is not possible here: the only real trigger is OOM inside
JSONParseitself, which a test cannot stage deterministically (constraining memory makes the earliertoJSONString()allocation fail first, which is a non-recoverable WTF crash, not a JS exception). The exception-check validator also cannot distinguish the two versions, because the old code'sreleaseAssertNoException()after the parse counts as checking the simulated throw. The test therefore passes before and after this change; it pins the function's behavior, which previously had no coverage at all, and the validator check guards the scope bookkeeping of the new code.The profiler is started without a directory argument because passing one currently segfaults; that is a separate bug with a separate fix in #32217.
Sibling sites
Review flagged the remaining instance of this pattern,
JSC__JSGlobalObject__generateHeapSnapshotinsrc/jsc/bindings/bindings.cpp(behindconsole.takeHeapSnapshot()). It is intentionally excluded: unlike the directbun:jschost functions fixed here and in #36857, its result feeds the console formatter through the Rust caller, so the reorder alone would pass an empty value into message formatting with an exception pending, and the exception would then surface under JSC'sconsoleProtoFuncTakeHeapSnapshot, whose scope performs no exception check after the client call. That needs caller-side handling and is tracked separately.