inspector: check exceptions in jsToInspectorValue - #376
Conversation
jsToInspectorValue calls getOwnPropertyNames (which reaches getOwnNonIndexPropertyNames and declares a ThrowScope) and then calls object.get() on each property without checking for an exception in between. Under validateExceptionChecks this aborts when the inspector backend dispatches with no JS on the stack, e.g. when InspectorConsoleAgent::enable() replays buffered console messages: the inner ThrowScope destructor simulates a throw because there is no topEntryFrame to attribute it to, and the next ThrowScope constructor in JSObject::get sees it unchecked. Declare a ThrowScope in the object branch and RETURN_IF_EXCEPTION after getIndex, getOwnPropertyNames, get, and the recursive calls. Give toInspectorValue (the JSLockHolder entry point) a TopExceptionScope and clear any exception before returning so the simulated throw does not leak to scope-free inspector callers.
|
Warning Review limit reached
Next review available in: 20 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical exception-scope hygiene fix following the established JSC pattern.
What was reviewed:
- Verified
TopExceptionScope/DECLARE_TOP_EXCEPTION_SCOPE/clearExceptionExceptTerminationAPI matches usage here (same pattern asJSMicrotask.cpp,JSPromise.cpp). - Checked all
toInspectorValuecallers (InjectedScript.cpp,InjectedScriptBase::makeCall/makeAsyncCall) already treat a null return as failure, so clearing and returningnullptris safe. - Confirmed
RETURN_IF_EXCEPTIONplacement covers each throwing site (getIndex,getOwnPropertyNames,get, and the recursive calls) named in thevalidateExceptionCheckstrace.
Extended reasoning...
Overview
This PR touches a single file, Source/JavaScriptCore/inspector/InjectedScriptBase.cpp, adding exception-scope discipline to jsToInspectorValue and its public entry point toInspectorValue. In the object branch it declares a ThrowScope and inserts RETURN_IF_EXCEPTION(scope, nullptr) after array.getIndex, getOwnPropertyNames, object.get, and each recursive call. At the entry point it adds a TopExceptionScope (after the existing JSLockHolder) and clears any non-termination exception before returning nullptr, so nothing leaks to inspector callers that have no exception scope of their own.
Security risks
None. This is defensive exception-checking that only changes behavior when a JS exception is already pending (or when validateExceptionChecks simulates one). No new data flows, no untrusted input handling, no auth/permissions surface. The inspector already serializes these values; the only difference is that on exception the function now bails cleanly instead of continuing with a potentially bogus JSValue or aborting under the exception validator.
Level of scrutiny
Low. The change is small (~25 lines net), purely additive, and follows an idiom used pervasively across the codebase — I confirmed identical DECLARE_TOP_EXCEPTION_SCOPE + scope.exception() + clearExceptionExceptTermination() sequences in runtime/JSMicrotask.cpp, runtime/JSPromise.cpp, and runtime/PinballCompletion.cpp. TopExceptionScope inherits exception() from ExceptionScope, so the if (scope.exception()) check compiles in both the verification-enabled and release variants. The VM& declaration was simply hoisted from the non-array object path to the top of the isObject() block so both sub-branches can share the scope.
Other factors
All call sites of toInspectorValue (InjectedScript::wrapObject and siblings in InjectedScript.cpp, plus makeCall/makeAsyncCall in this file) already null-check the result and fall back to an error string, so the new early-nullptr returns integrate without further changes. The PR description precisely matches the observed validateExceptionChecks abort trace and the fix is the canonical remedy for that class of failure. No outstanding reviewer comments; the only timeline entry is a CodeRabbit rate-limit notice.
Preview Builds
|
inspect.test.ts has pre-existing localhost-vs-[::1] failures in the gate environment and is ASAN-quarantined in CI, so the new test never ran there. Drop the preview-pin lint (the preview pin is called out in the PR body and in the webkit.ts comment; the PR will swap to the main sha once oven-sh/WebKit#376 merges).
1 participant
jsToInspectorValuecallsgetOwnPropertyNames(which reachesgetOwnNonIndexPropertyNamesand declares aThrowScope) and then callsobject.get()on each property without checking for an exception in between. UndervalidateExceptionChecksthis aborts when the inspector backend dispatches with no JS on the stack:The path that hits this in Bun is
InspectorConsoleAgent::enable()replaying bufferedConsoleMessages: each message callsInjectedScript::wrapObject, whose result is passed totoInspectorValue. Because there is notopEntryFrame(no JS on the stack during backend dispatch), the innerThrowScopedestructor ingetOwnNonIndexPropertyNamessimulates a throw, and the nextThrowScopeconstructor inJSObject::getsees it unchecked. The liveaddConsoleMessagepath (already enabled, called fromconsole.lognative) does not trip this because atopEntryFrameis present and the destructor skips the simulated throw.Declare a
ThrowScopein the object branch andRETURN_IF_EXCEPTIONaftergetIndex,getOwnPropertyNames,get, and the recursive calls. GivetoInspectorValue(theJSLockHolderentry point) aTopExceptionScopeand clear any non-termination exception before returning so nothing leaks to the scope-free inspector callers (InjectedScript::wrapObjectand friends), which already treat a null return as failure.