[JSC] WebAssembly.Exception.prototype.getArg: an index that is not an unsigned long is a TypeError, not a RangeError - #613
Conversation
… unsigned long is a TypeError, not a RangeError getArg(tag, index) declares index as [EnforceRange] unsigned long (WebAssembly/exception-handling#257, now in the WebAssembly/spec js-api). A WebIDL [EnforceRange] conversion that fails, because the value is NaN, an infinity, or has an integer part outside [0, 2^32 - 1], throws TypeError. Only an index that converts and is then past the payload is a RangeError. V8 (EnforceUint32 in wasm-js.cc) and SpiderMonkey (EnforceRangeU32, JSMSG_WASM_BAD_ENFORCE_RANGE) both throw TypeError. JSC threw TypeError too until 282520@main (bug 278413), which passed ErrorType::RangeError from getArg to toNonWrappingUint32() so that the imported WPT test wasm/jsapi/exception/getArg.tentative.any.js would pass. That WPT copy predates exception-handling#257: the copy of the same test in WebAssembly/spec asserts TypeError, V8 lists the WPT copy as an expected failure in test/wasm-js/wasm-js.status, and Firefox marks its "Getting out-of-range argument" subtest as an expected FAIL. This reverts the call-site half of 282520@main and restores the previous FAIL expectation for that subtest. toNonWrappingUint32() keeps its ErrorType parameter, unused for now, so the helper stays as upstream has it. * JSTests/wasm/js-api/exception-getarg-index-enforce-range.js: Added. * JSTests/wasm/v8/exceptions-api.js: (TestGetArg): Restore the getArg(tag, undefined) TypeError assertion with JSC's message. * LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/exception/getArg.tentative.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/exception/getArg.tentative.any.worker-expected.txt: * Source/JavaScriptCore/wasm/js/WebAssemblyExceptionPrototype.cpp: (JSC::JSC_DEFINE_HOST_FUNCTION):
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line spec-conformance fix with thorough test coverage.
What was reviewed:
- Confirmed
toNonWrappingUint32defaults toErrorType::TypeError(JSWebAssemblyHelpers.h:49), so dropping the explicitRangeErrorargument yields the WebIDL-mandated behavior for[EnforceRange];RETURN_IF_EXCEPTIONand the subsequent tag/index-past-payload checks are unchanged. - New test in
JSTests/wasm/js-api/follows existing directory convention, usesassert.js(throws on failure, no printing), and covers conversion ordering, empty payloads, and ToNumber propagation. - WPT expected-output flip to FAIL is intentional: the imported test still asserts
RangeError, which the PR shows is stale relative to the spec; V8/Firefox already expect this subtest to fail.
Extended reasoning...
Overview
The functional change is a single line in Source/JavaScriptCore/wasm/js/WebAssemblyExceptionPrototype.cpp: the index argument to WebAssembly.Exception.prototype.getArg is now converted via toNonWrappingUint32(globalObject, callFrame->argument(1)) without the explicit ErrorType::RangeError, so the helper falls back to its default ErrorType::TypeError. This matches WebIDL [EnforceRange] unsigned long semantics and how every other wasm JS API caller of the same helper already behaves. The rest of the host function — RETURN_IF_EXCEPTION, the tag-mismatch TypeError, and the index-past-payload RangeError — is untouched. Accompanying changes are a new regression test in JSTests/wasm/js-api/, an uncommented assertion in the imported V8 exceptions-api.js (with JSC's error message substituted), and two WPT -expected.txt files flipped from PASS to FAIL because the vendored WPT test still asserts the pre-spec-change RangeError.
Security risks
None. This changes only the constructor of the error object thrown when a JS caller passes an out-of-range or non-numeric index. There is no memory access, allocation sizing, JIT codegen, or privilege boundary involved; it is a pure host-function error-type adjustment.
Level of scrutiny
Low. The source change is one deleted argument reverting to a well-established default, and I verified the default in JSWebAssemblyHelpers.h. This is a host function (JSC_DEFINE_HOST_FUNCTION), so there are no per-tier LLInt/Baseline/DFG/FTL implementations to keep in sync. The new test lives alongside dozens of similar wasm/js-api/*.js tests, uses the shared assert.js (throws on failure, no logging), has no hot loops needing wasmTestLoopCount, and is clearly sub-200ms. The WPT expectation update is the standard WebKit way of tracking a stale imported test.
Other factors
.github/CODEOWNERS lists @ WebKit/jsc-reviewers for Source/JavaScriptCore and JSTests, but the file's own header states contributors do not "own" components — it is upstream WebKit's auto-reviewer-add list carried into this fork, not a gating ownership rule here. Given the change is a trivial, well-justified partial revert of an upstream tweak with comprehensive tests (the PR reports full wasm.yaml and Debug+ASan --validateExceptionChecks passes), approving is appropriate.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesThe PR updates ChangesException.getArg index validation
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Exception.getArg now reports invalid unsigned index conversions as TypeError while retaining RangeError for valid indices outside the payload; the change is ready to merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
2 participants
Problem
WebAssembly.Exception.prototype.getArg(tag, index)throwsRangeError: Expect an integer argument in the range: [0, 2^32 - 1]forindexvalues like-1,2 ** 32,NaNandundefined. The IDL is[EnforceRange] unsigned long index([js-api] Add missing EnforceRange to getArg argument. WebAssembly/exception-handling#257), so a failed conversion is aTypeError. V8 (EnforceUint32) and SpiderMonkey (JSMSG_WASM_BAD_ENFORCE_RANGE) throwTypeError.WebAssemblyExceptionPrototype.cpp:118, which passesErrorType::RangeErrortotoNonWrappingUint32(). Upstream 282520@main (bug 278413) added that so the imported WPTwasm/jsapi/exception/getArg.tentative.any.jswould pass. That WPT copy predates exception-handling#257: the same test in WebAssembly/spec assertsTypeError, V8 lists the WPT copy as an expected failure, and Firefox marks its "Getting out-of-range argument" subtest as an expected FAIL.Fix
getArgcallstoNonWrappingUint32()with the defaultTypeError. The helper keeps its parameter, soJSWebAssemblyHelpers.hstays identical to upstream. An index that converts but is past the payload is still aRangeError.getArg.tentative.any*-expected.txtfiles go back to the FAIL line they had before 282520@main.JSTests/wasm/v8/exceptions-api.jsgets back thegetArg(tag, undefined)assertion that was commented out on import.JSTests/wasm/js-api/exception-getarg-index-enforce-range.js(new) and the v8exceptions-api.jsfail on an unpatched Release JSCOnly build and pass patched in all 13 wasm modes. The wholeJSTests/wasm.yamlrun (17940 runs) passes. A Debug+ASan build passes the new test with--validateExceptionChecks=true.Background
[EnforceRange] unsigned long:NaNand the infinities throwTypeError, then the integer part must be in[0, 2^32 - 1]or it throwsTypeError. So-0.5and0.9are a valid0.toNonWrappingUint32()implements this, and every other wasm JS API caller (Memory and Table sizes, deltas, indices) already getsTypeErrorfrom it.getArgsteps run after that conversion: a tag mismatch is aTypeError, thenindex >= payload sizeis aRangeError.Notes
getArg.tentative.any.js:46,RangeErrortoTypeError, matching WebAssembly/spectest/js-api/exception/getArg.tentative.any.js). This PR is the minimal fork-side carry of the same change. V8's expectation is intest/wasm-js/wasm-js.status, Firefox's intesting/web-platform/meta/wasm/jsapi/exception/getArg.tentative.any.js.ini.WebAssembly.Memoryhand out a newSharedArrayBufferaftergrow(0), because Node 26.3 does. That was wrong: V8mainsince crrev.com/c/7660970 only refreshes the buffer of a shared memory whose length actually grew, SpiderMonkey'sWasmMemoryObject::refreshBufferdoes the same, and [threads][test][js-api] Add tests for growing shared memory WebAssembly/threads#248 adds the test "Growing shared memory by 0 does not change the buffer". JSC already behaves that way, so that part was dropped.