Repository navigation
TypedArray.prototype.with: keep a BigInt array's elements when argument coercion grows its buffer (WebKit bump for oven-sh/WebKit#601) - #42082
Conversation
…nt coercion grows its buffer (WebKit bump for oven-sh/WebKit#601)
|
Status Reproduced on bun 1.4.3 and on current const b = new ArrayBuffer(32, { maxByteLength: 64 });
const v = new BigInt64Array(b);
v.set([10n, 11n, 12n, 13n]);
console.log(v.with(0, { valueOf() { b.resize(40); return 1n; } }).join()); // bun: 1,0,0,0 node: 1,11,12,13
The engine fix is oven-sh/WebKit#601. This PR pins its preview build and must not merge before that PR lands; I will move the pin to the merged commit. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe change updates the WebKit build reference to a preview release and adds tests for ChangesWebKit release reference
Resizable typed-array behavior
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This updates WebKit for the TypedArray.prototype.with resizable-buffer fix and adds regression coverage. No concrete merge-blocking risk remains in the supplied evidence. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The automated review raised no findings, so the diff stays as is. The only open item is the merge order: oven-sh/WebKit#601 lands first, then I move |
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "2e2aa2290fac856d6f451ceacb58f7f5b44dd057"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-601-2b6703ca"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-601-2b6703ca, an ephemeral preview-PR release tag; merging this would leave main pointing at an artifact that disappears once the WebKit PR closes and that carries unmerged upstream commits. Fix: before merge, swap to the merged oven-sh/WebKit commit SHA (or its autobuild-<sha> tag) and verify prebuilt archives exist for every platform × flavor, per the Dependencies & vendoring rule and .claude/commands/upgrade-webkit.md. The PR description already flags this as a do-not-merge precondition; this comment tracks it as the merge blocker.
Extended reasoning...
.claude/docs/landing-prs.md (Dependencies & vendoring) states: "Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds) — swap to the merged upstream SHA and verify prebuilt artifacts exist for every platform × flavor before merge." scripts/build/download.ts:328 already warns that autobuild-preview-pr-* releases only exist while the WebKit PR is open, and .claude/commands/upgrade-webkit.md:34 says the preview tag must be bumped to the merge-commit's autobuild-<sha> before merging the bun PR. If this lands as-is and oven-sh/WebKit#601 is later closed/force-pushed, fresh checkouts and CI will fail to fetch WebKit prebuilts. The base branch pinned a merged SHA (2e2aa2290fac…), so this is a regression the diff introduces. The author notes the same in the PR body; per the vendoring rule it is still a hard merge blocker to track.
Verification: normal — acknowledged in diff: the PR description states "The pin is a preview tag, not a merge commit. Do not merge before oven-sh/WebKit#601 lands. I move the pin to the merged commit then." That claim is accurate but does not resolve the hazard; the pin must be swapped before merge. scripts/build/deps/webkit.ts:6 changes WEBKIT_VERSION from the merged SHA… | normal — acknowledged in diff:…
There was a problem hiding this comment.
Agreed, and intended. The preview tag is only here so CI can run the new test against the engine fix before oven-sh/WebKit#601 merges. Once it lands I push a commit that sets WEBKIT_VERSION to the merged 40-character SHA and check that the autobuild-<sha> release carries all prebuilt archives, then resolve this thread. Leaving it open until then as the merge blocker.
|
The review's one finding is the preview-tag pin at |
Problem
BigInt64Array.prototype.with(index, value)andBigUint64Array.prototype.withlose data when the receiver is a length-tracking view over a resizableArrayBufferor growableSharedArrayBufferand coercingindexorvaluegrows that buffer. The returned copy keeps the replaced element and has0in every other slot:ta.set([10n, 11n, 12n, 13n]); ta.with(0, { valueOf() { rab.resize(40); return 1n; } })gives1,0,0,0. Node and the spec give1,11,12,13. The ten Number element types, fixed-length views, and shrinking are correct.genericTypedArrayViewProtoFuncWith()(Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:2107). Its "length changed during coercion" loop copies elementkonly ifcanGetIndexQuickly(k), which isinBounds(k) && Adaptor::canConvertToJSQuickly. That flag isfalsefor the BigInt adaptors (making a JSValue allocates aJSBigInt), but the loop reads the native value and never makes a JSValue. Everyk != indexfell through to the0default.Fix
autobuild-preview-pr-601-2b6703ca, the preview build of [JSC] %TypedArray%.prototype.with: copy the elements of a BigInt array whose buffer grew during argument coercion WebKit#601, which checksinBounds(k)instead. That is the preconditiongetIndexQuicklyAsNativeValue(k)asserts.mainpast the current pin.test/js/bun/jsc/typed-array-with-resizable-buffer.test.ts(new) fails 16 of 120 on bun 1.4.3, exactly the BigInt64Array / BigUint64Array grow cases over both buffer kinds, and passes withbun bd testagainst this prebuilt. Also rantest/js/node/buffer.test.js,test/js/web/atomics.test.ts,test/js/bun/jsc/*, plus the JSC stress and test262TypedArray/prototype/withtests against a local build of the same WebKit commit.Background
%TypedArray%.prototype.withreadslenfirst, then runsToIntegerOrInfinity(index)andToNumber/ToBigInt(value), which can call user code that resizes the buffer. It then re-validates the index, creates the result with the originallen, and copies eachk != indexwithGet(O, k).new BigInt64Array(rab)with no explicit length) follows its buffer's byte length. After a grow everyk < lenis still in bounds, soGetreturns the stored element. After a shrink the tail reads asundefined: Number arrays storeNaN/0, BigInt arrays throw onToBigInt(undefined). JSC already threw there, so only the grow direction was wrong.memcpys the source when its length did not move across the coercions and only runs the per-element loop when it did, which is why a same-sizeresize()inside the hook was fine.Notes
JSTests/stress/v8-harmony-typed-array-with.js,TestParameterConversionGrows) takes this path for every element type but never writes to the array first, so the zeros compared equal. test262 has no case that grows the buffer inside the coercion with non-zero contents either; both suites pass before and after.mainhas the same line.[policy-decision:webkit] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The BigInt path of
%TypedArray%.prototype.withre-read the length of a length-tracking view after coercingindexandvalue, and when a coercion side effect grew the underlying resizable or growable buffer it allocated a result of the new, larger length but never copied the source elements into it, writing only the replacement element and leaving every other slot zero-initialized. The fix copies the source view's contents into the freshly allocated result before storing the replacement element, matching the non-BigInt path and the spec's requirement that every element atk != indexb…