node:inspector: emit full RemoteObject, stackTrace, and count/time*/assert/dirxml/clear for in-process Session consoleAPICalled - #35743
Conversation
…ssion consoleAPICalled, hook count/time/assert/dirxml/clear
The in-process Session's Runtime.consoleAPICalled mirroring serialized every
non-primitive argument as an opaque {type:'object', description:'[object Foo]'}
string with no subtype/className/preview/objectId, never attached a stackTrace,
and never emitted events for console.count/time/timeLog/timeEnd/assert/dirxml/
clear, so consumers that hook consoleAPICalled (APM SDKs, loggers) saw nothing
useful for objects and nothing at all for half the console surface.
The fix rebuilds toRemoteObject into a proper CDP RemoteObject builder
(util.types + JSC intrinsics for subtype detection; className via the
constructor chain; per-subtype description; a bounded property/entry preview;
synthetic objectId for shape parity), captures a CDP-shaped stackTrace per
event via Error.captureStackTrace with a prepareStackTrace that emits
{functionName, scriptId, url, lineNumber, columnNumber} frames, and adds hooks
for assert/dirxml/clear plus count/time/timeLog/timeEnd backed by local
SafeMap-tracked counters and timers so the 'label: N' and 'label: X ms'
strings match what V8's inspector emits.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesRuntime console events now produce richer CDP-compatible RemoteObjects, previews, object IDs, classifications, and stack traces. Specialized hooks add Runtime console events
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 7
🤖 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 `@src/js/node/inspector.ts`:
- Line 497: Reduce the stack-trace limit assigned in the hooked console-call
path around Error.stackTraceLimit to approximately 30, or reuse the existing
Error.stackTraceLimit when it is already configured, instead of forcing 200 on
every call. Preserve the Runtime domain’s stack reporting while avoiding
unnecessary CallSite and CDP frame creation.
- Line 198: Update src/js/node/inspector.ts at lines 198-198, 331-331, 340-340,
and 465-465 to destructure each repeatedly accessed property once before its
guard: use descriptor.value for the callable check and name read, info.subtype
for the entry assignment, p.subtype for the out assignment, and info.subtype for
the remote assignment. Preserve the existing guard and assignment behavior while
eliminating duplicate conditional property access.
- Around line 490-511: Update captureCDPStackTrace so every access to
Error.prepareStackTrace, including saving and restoring its value, is protected
by the existing best-effort error handling. Ensure throwing getters, setters, or
non-writable properties cannot escape the function or make console logging
throw, while preserving normal stack-trace capture and restoration behavior.
- Around line 520-541: Update emitConsoleAPICalled to return immediately when
runtimeEnabledSessions has no sessions, before capturing the timestamp or stack
trace. Preserve per-session isolation by cloning the captured stack-trace data,
including a distinct callFrames array, when assigning params.stackTrace for each
session; keep args rebuilding and existing message delivery behavior unchanged.
- Around line 208-210: Update truncate to avoid emitting a lone surrogate when
the length limit cuts through a surrogate pair: after slicing at
MAX_DESCRIPTION_LENGTH, remove the trailing high surrogate if present before
appending the ellipsis. Preserve the existing behavior for strings within the
limit and for truncation at non-surrogate boundaries.
In `@test/js/node/inspector/inspector-profiler.test.ts`:
- Around line 729-823: Extend the concurrent test around collect to log null,
undefined, bigint, symbol, -0, NaN, and positive/negative infinity, then assert
each primitive’s type, description, and applicable unserializableValue according
to toRemoteObject behavior. Add two distinct object arguments and verify their
objectId values are strings and differ, while preserving the existing
object-shape assertions.
- Around line 866-906: Remove the redundant test-body comments `// count`, `//
assert`, `// dirxml`, and the value-label comments that merely describe the
following assertions. Keep comments documenting non-obvious CDP behavior and the
hostile-value rationale, including the explanations near timeLog/timeEnd and
hostile arguments.
🪄 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: 558b4a42-5926-4fea-bc16-f6334bc3681a
📒 Files selected for processing (3)
src/js/node/inspector.tstest/js/node/inspector/inspector-profiler.test.tstest/js/node/inspector/inspector.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…guard every Error.prepareStackTrace/stackTraceLimit touch, clone callFrames per session, lower stack limit to 30, use captured ArrayPrototypeSlice, surrogate-safe truncate
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ssert(false) args, spread-free timeLog hook
…th pass 71/71 locally)
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/inspector.ts:654-660—for (const method of CONSOLE_API_SPECIAL)iterates a plain Array via the iterable protocol, so a tamperedArray.prototype[Symbol.iterator]makessession.post("Runtime.enable")throw after the firstfor...inloop has populatedhookedConsoleMethods— a retry then early-returns onhookedConsoleMethods.length > 0and the assert/count/time* hooks are never installed. This is another sibling of thetimeLogspread comment above (sameArray.prototype[Symbol.iterator]tamper class); use an index loop likeremoveConsoleHooks()does, or makeCONSOLE_API_SPECIALa null-proto record and iterate withfor...inlike the loop directly above.Extended reasoning...
What the bug is
installConsoleHooks()iterates the new special-method list with a plainfor...ofover an ordinary Array literal:for (const method of CONSOLE_API_SPECIAL) { const original = consoleObject[method]; ... }
for...ofon an Array evaluatesGetIterator→GetMethod(arr, @@iterator)→Array.prototype[Symbol.iterator], which is user-tamperable. This is a distinct sibling site of thetimeLog...extraspread already flagged in the open comment at line 641 — same tamper class, different location, and a different (arguably worse) failure mode: instead of making a singleconsole.timeLog()throw, it corrupts the module-level hook-installation state.Code path that triggers it
- User tampers
Array.prototype[Symbol.iterator](deletes it, or replaces it with a throwing function) before enabling Runtime. session.post("Runtime.enable")→#handleMethodrunsruntimeEnabledSessions.add(this)first, theninstallConsoleHooks().- The first loop —
for (const method in CONSOLE_API_TYPES)— is iterator-safe (null-proto record,for...inuses[[OwnPropertyKeys]], not the iterable protocol). It succeeds and populateshookedConsoleMethodswith the pass-through hooks forlog/error/… - The second loop evaluates
CONSOLE_API_SPECIAL[Symbol.iterator]()→ looks up onArray.prototype→ throws. post()callsthis.#handleMethod(...)outside any try/catch, so the exception surfaces synchronously to the user'ssession.post("Runtime.enable")call (regardless of whether a callback was passed).
Why this leaves partial state
runtimeEnabledSessionsalready contains the session (added before the throw).hookedConsoleMethods.length > 0(populated by the first loop).- The special hooks (
assert/count/countReset/time/timeLog/timeEnd) are never installed.
If the user retries
session.post("Runtime.enable"),installConsoleHooks()early-returns onif (hookedConsoleMethods.length > 0) return;— so the special hooks stay missing until every session disconnects andremoveConsoleHooks()clears the array. The pass-through hooks work, butcount/time*/assertsilently emit no CDP events for the rest of the session.Why existing safeguards don't help
The file explicitly hardens against this class elsewhere:
SafeSetfor theruntimeEnabledSessionsfor-of (with a comment stating exactly why),for...inover the null-protoCONSOLE_API_TYPESfor the sibling loop directly above, an index loop inremoveConsoleHooks()directly below, and the capturedArrayPrototypeSliceadded in 950aaae for theasserthook. This new loop is the only iteration ininstallConsoleHooks()that dispatches through a tamperable prototype method, and it was introduced by this PR.(One caveat on the "regression" framing:
installConsoleHooks()was never fully hardened —hookedConsoleMethods.push(...)already dispatches throughArray.prototype.pushbefore this PR — so this is more accurately a new internal inconsistency in new code than a strict behavioral regression.)Step-by-step proof
delete Array.prototype[Symbol.iterator]; const s = new (require("node:inspector").Session)(); s.connect(); s.post("Runtime.enable"); // throws TypeError: undefined is not a function (@@iterator)
At the throw point:
runtimeEnabledSessions.size === 1,hookedConsoleMethods.length === 14(the pass-through set),console.count === <original native>. A subsequents.post("Runtime.enable")returns{}(early-return), andconsole.count("x")never firesRuntime.consoleAPICalled.Impact
Nit — the trigger is deliberate exotic tampering of
Array.prototype[Symbol.iterator]beforeRuntime.enable, which is outside the file's stated invariant ("console.* itself never throws from the hook" — this issession.post()throwing at setup, not aconsole.*call), and the resulting state is graceful degradation (log/error/etc. still emit; only the special methods are silent). But per REVIEW.md's "Fix the whole class in the same PR — grep for every sibling site sharing the pattern", this belongs alongside thetimeLogfix, and it's inconsistent with both neighboring loops.Fix
Either an index loop:
for (let i = 0; i < CONSOLE_API_SPECIAL.length; i++) { const method = CONSOLE_API_SPECIAL[i]; ... }
or make
CONSOLE_API_SPECIALa null-proto record and iterate withfor...in, matchingCONSOLE_API_TYPESdirectly above. - User tampers
-
🟡
src/js/node/inspector.ts:674-675— ClearingconsoleCounts/consoleTimersonRuntime.disableresets the shadow state while Bun's native C++ console counters/timers persist, so after disable→re-enable the CDP event args diverge from stdout —console.count('x')emits'x: 1'while native prints'x: 3', and atimeEndfor a timer started before disable emits no event at all. V8 keepsm_counterMap/m_timerMapon the per-contextV8Console(not the session), so Node never resets onRuntime.disable; dropping the two.clear()calls preserves parity across the disable→(nothing)→re-enable case for free.Extended reasoning...
What the bug is
removeConsoleHooks()— called when the last Runtime-enabled session postsRuntime.disableor disconnects — clears the shadowconsoleCounts/consoleTimersmaps:hookedConsoleMethods.length = 0; consoleCounts.clear(); consoleTimers.clear();
But Bun's native C++ console state (the counters/timers these maps shadow — see the comment on line 570: "Shadow counters/timers parallel to Bun's native C++ console state, which is not readable from JS") is not cleared by
Runtime.disable. So after a disable → re-enable cycle, the shadow restarts from zero while native retains its counters, and the emittedRuntime.consoleAPICalledargs no longer match what the underlyingconsole.count/console.timeEndactually printed to stdout.Step-by-step proof
Counter divergence:
s.post('Runtime.enable')→installConsoleHooks()runs, shadow maps empty.console.count('x')→ shadow=1, native=1; CDP event args['x: 1'], stdoutx: 1✓console.count('x')→ shadow=2, native=2; CDP event args['x: 2'], stdoutx: 2✓s.post('Runtime.disable')→ last session drops →removeConsoleHooks()→consoleCounts.clear().s.post('Runtime.enable')→ hooks reinstalled, shadow empty, native still holdsx → 2.console.count('x')→ shadow computes(undefined ?? 0) + 1 = 1, native increments to 3; CDP event args['x: 1'], stdoutx: 3✗
Timer event dropped entirely:
- enable →
console.time('t')→ both shadow and native record the start. - disable →
consoleTimers.clear(). - enable →
console.timeEnd('t')→consoleTimers.get('t') === undefined→ theif (start !== undefined)guard skipsemitConsoleAPICalledentirely, notimeEndevent is emitted, but native still printst: <elapsed> ms.
Why this differs from Node
In V8 the console counters and timers live on the per-context
V8Consoleobject (m_counterMap/m_timerMapinv8/src/inspector/v8-console.cc), not on the inspector session.Runtime.disabletears down session-scoped state only, so those maps survive. Running the sequence above in Node, step 6 emits{args:[{value:'x: 3'}]}matching stdout, and thetimeEndafter a disable/re-enable cycle still fires with the original start time.Why removing the clear is a strict improvement
The shadow can already drift from native if
console.count/console.timeare called between disable and re-enable — that gap is unavoidable given native state is unreadable from JS. But clearing makes it drift even when nothing happens between disable and re-enable, which is the one case where parity would otherwise be preserved for free. For any sequence of operations, drift-without-clear ≤ drift-with-clear: calls between disable and enable increment native but not shadow either way, and not clearing preserves the pre-disable count that would otherwise be discarded. (The one contrary case — acountResetbetween disable and enable — is symmetrically approximate either way, so it doesn't argue for keeping the clear.) If unbounded growth is a concern, note that nativeBun__ConsoleObject__count/timegrow the same way, so parity argues for keeping the shadow in step.Impact and fix
Nit — the trigger (toggling the Runtime domain while count/time state is live) is niche, nothing crashes, and the shadow design is inherently approximate. The fix is a two-line deletion of
consoleCounts.clear()andconsoleTimers.clear()fromremoveConsoleHooks().
…o native, per-session frame object clone, keep shadow maps across Runtime.disable
|
Diff is ready. Local verification at 1e17700: CI (build 81231): four Gate: the internal gate reports one ASAN-with-fix failure but the output is truncated before the failing test name; the junit it wrote to disk is from the later release lane and shows 0 failures. Given the four new subprocess tests here do not bind ports and pass 3/3 locally, this looks like one of the pre-existing |
2 participants
Repro
Cause
The in-process
Session's console hook insrc/js/node/inspector.tsbuilt each argument with a fall-throughObject.prototype.toString.call(arg)description and nothing else, never captured a stack, and only hooked the methods whose CDP args are the raw JS args (log/error/trace/...). This is a separate implementation from the WebSocket CDP path insrc/js/internal/inspector/cdp.ts, which gets subtype/className/preview/objectId/stackTrace from JSC'sConsole.messageAdded.Fix
classifyObject()detects subtype/className vianode:util/typespredicates and JSC intrinsics (so a hostile Proxy or getter cannot derail classification), builds a per-subtype description (Array(3),Map(1),Uint8Array(2), the error's stack, the regexp's source, ...), andbuildPreview()fills a boundedpreview.properties/preview.entries(5-item cap,overflowset past that). Objects and functions get a syntheticobjectIdso the shape matches V8; the in-process Session has noRuntime.getPropertiesbackend to dereference it, but consumers commonly test for the field to tell primitives from objects.captureCDPStackTrace()usesError.captureStackTracewith aprepareStackTracethat emits CDP{functionName, scriptId, url, lineNumber, columnNumber}frames (0-based), starting above the hook so the top frame is the user's call site.dirxml/clearare added to the pass-through table;assertemits only on a falsy condition;count/countReset/time/timeLog/timeEndget bespoke hooks backed bySafeMap-tracked shadow counters/timers so the"label: N"/"label: X ms"strings match what V8's inspector emits.Verification
bun bd test test/js/node/inspector/passes 71/71. Four new subprocess tests ininspector-profiler.test.tscover the RemoteObject shape across array/error/map/set/typed-array/date/regexp/promise/plain-object/function, the stackTrace frame shape, the count/time*/assert/dirxml/clear event stream, and robustness against revoked proxies / null-prototype objects. Withsrc/js/node/inspector.tsreverted tomainthose four fail on subtype/stackTrace/missing-events while the rest of the suite still passes.Two existing
inspector.test.tscases are updated to match the new behavior: the prototype-pollution guard now checksconsole.profile(sincecountis legitimately hooked), and the revoked-proxy case now asserts the event is delivered withsubtype:"proxy"and no process warning (the builder handles it instead of throwing).Related: #35736 fixes the WebSocket CDP path's timestamp unit and its count/time forwarding in
ConsoleObject.cpp; this PR is the in-processSessionpath and does not overlap at the file level.