Inspector: handle BigInt/Symbol in jsToInspectorValue instead of asserting - #384
Inspector: handle BigInt/Symbol in jsToInspectorValue instead of asserting#384robobun wants to merge 1 commit into
Conversation
…rting
Runtime.evaluate / Runtime.callFunctionOn with returnByValue:true on a
result that contains a BigInt or Symbol (for example [1n], {a:1n}, or
Symbol()) reached the ASSERT_NOT_REACHED at the end of
jsToInspectorValue, aborting an assertions build. Release builds fell
through to nullptr and the caller reported the unrelated 'Object has too
long reference chain' message.
Return nullptr explicitly for BigInt and Symbol (they have no JSON
representation), and replace the misleading depth-limit message with the
same 'Object couldn't be returned by value' wording V8's inspector uses,
which is accurate for both the depth limit and the unserializable-primitive
cases.
WalkthroughChangesInspector value conversion
🚥 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: 1
🤖 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 `@Source/JavaScriptCore/inspector/InjectedScriptBase.cpp`:
- Around line 69-74: Add regression tests for the non-JSON handling in
InjectedScriptBase, covering direct 1n, direct Symbol(), and nested [1n] values
with returnByValue enabled. Exercise synchronous and asynchronous
Runtime.evaluate and Runtime.callFunctionOn flows, plus
Debugger.evaluateOnCallFrame where supported, and assert the "Object couldn't be
returned by value" error while confirming execution completes without reaching
ASSERT_NOT_REACHED.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 904ce0ce-dc1a-4f63-8543-0825b32e83ff
📒 Files selected for processing (1)
Source/JavaScriptCore/inspector/InjectedScriptBase.cpp
| // BigInt and Symbol have no JSON representation. They can reach this point | ||
| // when Runtime.evaluate / callFunctionOn is invoked with returnByValue on a | ||
| // result that holds one (e.g. `[1n]` or `Symbol()`), so report the value as | ||
| // not representable rather than hitting ASSERT_NOT_REACHED below. | ||
| if (value.isBigInt() || value.isSymbol()) | ||
| return nullptr; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add regression coverage for non-JSON values.
Add or verify tests for direct 1n, direct Symbol(), and nested values such as [1n] with returnByValue: true. Cover the synchronous and asynchronous paths, including Runtime.evaluate, Runtime.callFunctionOn, and Debugger.evaluateOnCallFrame where applicable. Assert "Object couldn't be returned by value" and verify that execution does not reach ASSERT_NOT_REACHED.
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 69-69: The function 'inspectorEnvironment' is never used.
(unusedFunction)
🤖 Prompt for 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.
In `@Source/JavaScriptCore/inspector/InjectedScriptBase.cpp` around lines 69 - 74,
Add regression tests for the non-JSON handling in InjectedScriptBase, covering
direct 1n, direct Symbol(), and nested [1n] values with returnByValue enabled.
Exercise synchronous and asynchronous Runtime.evaluate and
Runtime.callFunctionOn flows, plus Debugger.evaluateOnCallFrame where supported,
and assert the "Object couldn't be returned by value" error while confirming
execution completes without reaching ASSERT_NOT_REACHED.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped fix that stops the inspector from asserting on BigInt/Symbol under returnByValue and gives a clearer error string.
What was reviewed:
- Verified
JSValue::isBigInt()covers both BigInt32 and HeapBigInt, andisSymbol()guards onisCell()— safe to call at that point in the type ladder. - Placement before
isObject()is correct; these were the only primitive kinds that previously fell through toASSERT_NOT_REACHED. - Confirmed nothing else in the tree matches the old "Object has too long reference chain" string, so the message change is self-contained.
Extended reasoning...
Overview
The PR touches a single file, Source/JavaScriptCore/inspector/InjectedScriptBase.cpp, with two changes: (1) an early return nullptr in jsToInspectorValue when the value is a BigInt or Symbol, and (2) rewording the error string produced by makeCall / makeAsyncCall when toInspectorValue returns null. Net effect: an assertions build no longer aborts when Runtime.evaluate with returnByValue: true encounters a BigInt/Symbol, and release builds report a message that actually describes the failure.
Security risks
None. This is inspector-protocol serialization code that runs only when a debugger is attached. The new branch does no allocation or coercion — it just returns nullptr, which was already the release-build behavior via the ASSERT_NOT_REACHED(); return nullptr; fallthrough. The error-string change is a literal constant.
Level of scrutiny
Low. The diff is ~10 lines, mechanical, and confined to a debugging/inspector path. The release-build semantics for the BigInt/Symbol case are unchanged (still nullptr); only the debug-build assert and the user-facing error text differ. I checked JSCJSValueCell.h to confirm isBigInt() handles both the BigInt32 tag and heap BigInts and that isSymbol() is a null-safe cell-type check, so the new arm slots correctly between the existing primitive checks and the isObject() branch.
Other factors
A repo-wide grep found no other consumers of the old "Object has too long reference chain" string, so nothing depends on the exact wording. The new message matches V8's inspector wording per the PR description, which is a reasonable choice for tooling compatibility. No prior review comments or bot findings on this PR.
Preview Builds
|
1 participant
What
Runtime.evaluate/Runtime.callFunctionOn/Debugger.evaluateOnCallFramewithreturnByValue: trueon a result that holds a BigInt or Symbol reachedASSERT_NOT_REACHEDinInspector::jsToInspectorValue, aborting an assertions build. Release builds fell through tonullptrand the caller reported the unrelated "Object has too long reference chain (must not be longer than 1000)" error.Reproduces with any of:
[1n],({a:1n}),Symbol("s"),({b:Symbol()}). A bare top-level1nis fine becauseInjectedScriptSource.jsnever stores it onRemoteObject.value.Fix
isBigInt() || isSymbol()arm tojsToInspectorValuethat returnsnullptr(unserializable) instead of falling intoASSERT_NOT_REACHED.makeCall/makeAsyncCallwith "Object couldn't be returned by value", matching V8's inspector wording and accurate for both the depth-limit and the unserializable-primitive cases.Repro
Assertions build before:
SHOULD NEVER BE REACHEDinInjectedScriptBase.cpp:96, SIGABRT.Release before:
{"error":{"code":-32000,"message":"Object has too long reference chain ..."}}.After:
{"error":{"code":-32000,"message":"Object couldn't be returned by value"}}on both, debuggee stays alive.