inspector: release throw scope before tail-calling impl in injected-script prototype host functions - #328
Conversation
…cript prototype host functions
Every host function in JSInjectedScriptHostPrototype.cpp and
JSJavaScriptCallFramePrototype.cpp declares a ThrowScope (for the
dynamicDowncast type error) and then tail-calls into a
JSInjectedScriptHost / JSJavaScriptCallFrame member that itself
declares a ThrowScope. Under validateExceptionChecks=1 this aborts
with 'Unchecked JS exception' because the outer scope is never
released before the inner one is created.
Observed in Bun with any Runtime.evaluate over an inspector
connection:
ERROR: Unchecked JS exception:
This scope can throw a JS exception: evaluateWithScopeExtension @ JSInjectedScriptHost.cpp:120
But the exception was unchecked as of this scope: jsInjectedScriptHostPrototypeFunctionEvaluateWithScopeExtension @ JSInjectedScriptHostPrototype.cpp:275
Wrap every tail return in RELEASE_AND_RETURN(scope, ...) so the
outer scope is released before the callee may create its own.
|
Warning Review limit reached
Next review available in: 8 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 (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical application of the standard RELEASE_AND_RETURN idiom to 31 identical tail-call sites.
What was reviewed:
- Confirmed
RELEASE_AND_RETURN(scope, expr)expands toscope.release(); return expr;(ExceptionScope.h:151) — releasing before the callee runs is the correct fix for thevalidateExceptionChecks=1unchecked-scope abort. - Each site has no throwing work between the
dynamicDowncastnull check and the tail call, so releasing early cannot mask a real unchecked exception. - Pattern matches ~400 existing uses across JSC; no runtime behaviour change in release builds.
Extended reasoning...
Overview
This PR modifies two inspector prototype files (JSInjectedScriptHostPrototype.cpp and JSJavaScriptCallFramePrototype.cpp), rewriting 31 host-function tail returns from return JSValue::encode(castedThis->foo(...)) to RELEASE_AND_RETURN(scope, JSValue::encode(castedThis->foo(...))). Every changed function has the identical shape: declare a ThrowScope, dynamicDowncast the receiver, throw a TypeError on failure, otherwise tail-call into the impl.
Security risks
None. The change only affects JSC's debug-time exception-scope validation bookkeeping. scope.release() marks the outer scope as intentionally not checking for exceptions because responsibility is delegated to the callee; the actual VM exception state and propagation are untouched. No new inputs, no new control flow, no allocation changes.
Level of scrutiny
Low. This is a purely mechanical transformation applying a well-established JSC macro (405 existing occurrences of this exact RELEASE_AND_RETURN(scope, JSValue::encode(...)) form across 97 files). The macro definition in ExceptionScope.h is trivial: release the scope, then return the expression. In release builds where exception-check validation is compiled out, this is effectively a no-op. In debug builds with validateExceptionChecks=1, it fixes a false-positive abort that fires when the callee (e.g. evaluateWithScopeExtension) declares its own ThrowScope.
Other factors
- The PR applies the change uniformly to all 31 sites (not just the ~13 whose callees currently declare a
ThrowScope), which is the right call for consistency and future-proofing — releasing before a callee that doesn't throw is harmless. - I checked that in every function nothing can throw between
DECLARE_THROW_SCOPEand the tail call other than the already-handledthrowVMTypeErrorbranch, so the release cannot hide a genuine unchecked exception. - No prior human reviews or outstanding comments on the PR; the only timeline entry is a coderabbit rate-limit notice.
- The motivation (unblocking
validateExceptionChecks=1for Bun's inspector tests) is clearly stated and the fix is the canonical one.
… too test/expectations.txt quarantined test/cli/inspect/inspect.test.ts on ASAN as a TIMEOUT ever since ASAN CI was enabled (d8a69d6). That timeout is this exact bug: the describe("websocket") tests send Runtime.evaluate with bunEnv (which inherits the runner's BUN_JSC_validateExceptionChecks=1 on ASAN), the inspectee SIGABRTs before replying, and the unawaited .resolves assertion then hangs. With the WebKit fix those tests pass, so remove the quarantine. ENABLE_EXCEPTION_SCOPE_VERIFICATION is (ASSERT_ENABLED || ASAN_ENABLED), and the build forces assertions on for ASAN, so the explicit regression test should run on release-asan as well as debug; gate on (!isDebug && !isASAN) instead of just !isDebug. Also wire the post-open WebSocket error event to rejection, and add a source-lint test that fails when WEBKIT_VERSION is an autobuild-preview-* tag so a preview pin cannot be merged to main by accident (it is the merge gate for this PR until oven-sh/WebKit#328 lands and the pin is swapped to the merged sha).
Preview Builds
|
2 participants
Every host function in
JSInjectedScriptHostPrototype.cppandJSJavaScriptCallFramePrototype.cppdeclares aThrowScope(for thedynamicDowncasttype error) and then tail-calls into aJSInjectedScriptHost/JSJavaScriptCallFramemember. Ten of those members (evaluateWithScopeExtension,getInternalProperties,iteratorEntries,queryInstances,queryHolders,weakMapEntries,weakSetEntries,getOwnPrivatePropertySymbols,getOwnPrivatePropertyMethods,isPromiseRejectedWithNativeGetterTypeError, plusJSJavaScriptCallFrame::evaluateWithScopeExtension/scopeDescriptions/scopeChain) declare their ownThrowScope, so undervalidateExceptionChecks=1the firstRuntime.evaluateover an inspector connection aborts:Wrap every
return JSValue::encode(castedThis->...)inRELEASE_AND_RETURN(scope, ...)so the outer scope is released before the callee may create its own. Applied uniformly to all 31 sites in both files to keep the pattern consistent; no behavioural change for callees that do not declare a scope.The same code is present in upstream WebKit.
Needed for oven-sh/bun#31823, which currently exempts its
inspector.open()fixture from exception-check validation to work around this.