process: don't cache Exception cell as process.nextTick on init failure#30285
process: don't cache Exception cell as process.nextTick on init failure#30285robobun wants to merge 2 commits into
Conversation
When process.nextTick is first accessed while the stack is exhausted, the lazy initializer's profiledCall returns the JSC::Exception cell (from Interpreter::executeCallImpl's throwStackOverflowError path). That cell was being returned from the PropertyCallback and cached via putDirect as the value of process.nextTick. Calling that cell later reaches errorDescriptionForValue -> JSCell::toStringSlowCase which asserts isSymbol() || isHeapBigInt() in debug builds (and throws a misleading symbol-coercion TypeError in release). Use the exception-catching profiledCall overload so the callback never throws (PropertyCallbacks are assumed not to) and return jsUndefined() instead of the Exception cell on failure.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChanged Changesprocess.nextTick Safe Exception Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 4:06 PM PT - May 5th, 2026
❌ @autofix-ci[bot], your commit 40c002a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30285That installs a local version of the PR into your bun-30285 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix looks correct and well-tested, but since it changes process.nextTick's lazy-init failure semantics (silently swallowing the exception and permanently caching undefined), it'd be good to have a maintainer confirm that tradeoff is preferred over alternatives like reporting the swallowed error.
Extended reasoning...
Overview
This PR modifies Process::constructNextTickFn in src/jsc/bindings/BunProcess.cpp (~6 lines) and adds a regression test. It switches the lazy PropertyCallback initializer for process.nextTick to the NakedPtr<Exception>& overload of JSC::profiledCall, so that if the JS initializeNextTickQueue builtin throws (e.g. stack overflow on first access), the exception is caught and jsUndefined() is returned instead of leaking the raw JSC::Exception cell as the cached property value. The non-object fallback now also returns jsUndefined() rather than the raw value. The happy path (initializer succeeds, returns a function) is unchanged.
Security risks
None identified. This is purely an error-path hardening change in JSC bindings; no new inputs, parsing, auth, or external surface area.
Level of scrutiny
Medium-high. process.nextTick is one of the hottest and most fundamental Node APIs in the runtime, and this is its one-shot lazy initializer. The diff is small and the pattern (NakedPtr<Exception> + profiledCall) is used widely elsewhere in src/jsc/bindings, and the C++-side caller Process::queueNextTick already handles a null m_nextTickFunction / non-object nextTick gracefully. However, the new behavior permanently reifies process.nextTick as undefined after a transient init failure and silently discards the underlying exception. The PR description convincingly argues this is forced by JSC's PropertyCallback invariant (cannot throw without tripping EXCEPTION_ASSERT in LLInt get_by_id), but whether to additionally surface/log the swallowed error, or whether the permanent-undefined degradation is acceptable, is a judgment call a maintainer should sign off on.
Other factors
- Root-cause analysis in the PR description is thorough and matches the code; verified against
reifyStaticPropertysemantics and the existingqueueNextTickfallback atBunProcess.cpp:3815-3821. - Regression test exercises the exact fuzzer scenario in a child process and asserts no crash / no Exception-cell leak.
- robobun reports a
build-cppfailure on macOS aarch64 for the pre-autofix commit; the C++ change itself looks syntactically standard, so this may be infra/flake, but it's another reason to have a human glance before merge. - No prior
claude[bot]review on this PR.
|
CI failures on build #51800 are unrelated pre-existing flakes:
The new |
|
Closing in favor of #30441, which fixes the same root cause but reports the swallowed exception via |
1 participant
What does this PR do?
Fixes a debug assertion failure at
JSCell.cpp(268)(isSymbol() || isHeapBigInt()) found by Fuzzilli.Root cause
process.nextTickis a lazyPropertyCallback. On first access,constructNextTickFnruns the JSinitializeNextTickQueuebuiltin viaJSC::profiledCall.If that call fails — e.g. the first ever access to
process.nextTickhappens while the stack is already exhausted —Interpreter::executeCallImplreturns theJSC::Exception*cell itself as aJSValue(viareturn throwStackOverflowError(...)/RETURN_IF_EXCEPTION_WITH_TRAPS_DEFERRED(scope, scope.exception())).constructNextTickFnhad no exception check and returned that value, whichreifyStaticPropertythenputDirect'd as the cached value ofprocess.nextTick.On the next call, JS tries to invoke that raw
Exceptioncell (JSTypeCellType, not an object/string/symbol/bigint). The call-IC not-a-function path ends up inerrorDescriptionForValue→JSValue::toString→JSCell::toStringSlowCase, which assertsisSymbol() || isHeapBigInt()in debug builds and throws a misleading "Cannot convert a symbol to a string" TypeError in release builds.Minimal repro:
Fix
Use the
NakedPtr<Exception>&overload ofprofiledCallso the initializer's exception is caught (lazyPropertyCallbacks are assumed by JSC not to throw — leaving the exception pending also tripsEXCEPTION_ASSERT(!scope.exception() || !hasSlot)in LLIntget_by_id). On failure, returnjsUndefined()instead of theExceptioncell so calling it later produces a normal "is not a function" TypeError rather than a crash.How did you verify your code works?
TypeErrorinstead of aborting on the assertion, in both JIT anduseJIT=0modes.test/js/node/process/process-nexttick-stack-overflow.test.tswhich fails against currentmain(process.nextTickleaks as a non-function value) and passes with this change.process-nexttick.test.jssuite still passes.BUN_JSC_validateExceptionChecks=1passes on the repro.