Repository navigation
Upgrade WebKit: keep frozen, sealed and non-extensible array elements in the ArrayStorage vector #44388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
13
commits into
main
Choose a base branch
from
robobun/cb1239ce/frozen-array-vector
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Upgrade WebKit: keep frozen, sealed and non-extensible array elements in the ArrayStorage vector #44388
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
0546871
Keep frozen, sealed and non-extensible array elements in the ArraySto…
robobun f1f31e0
Pin WebKit to the 98c438b4 preview
robobun 0ce8af6
Pipe stderr in the Array.prototype test
robobun 1dc6a9c
[autofix.ci] apply automated fixes
autofix-ci[bot] 539d598
Test: cover holey, length-extended and sparse-map inputs, both protot…
robobun 6982ff6
[autofix.ci] apply automated fixes
autofix-ci[bot] 066a31a
Test: assert the length after every lock, the append rejection, and r…
robobun d778a4a
Test: assert isFrozen / isSealed per lock, the empty array, and the e…
robobun 01ad540
Test: the blank array case uses new Array()
robobun 3a6e848
Test: an empty array has no shape to assert, keep the semantic checks
robobun 1975773
Test: index and length writes on a locked empty array and on a frozen…
robobun 183647e
Test: use Reflect.set for the frozen Array.prototype writes
robobun e492787
[autofix.ci] apply automated fixes
autofix-ci[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,332 @@ | ||
| import { describeArray, describe as describeObject } from "bun:jsc"; | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| // Object.freeze / seal / preventExtensions and a read-only "length" keep the elements of an | ||
| // array in the ArrayStorage vector (oven-sh/bun#44305). Before the fix every element moved | ||
| // into the sparse map: the vector length dropped to 0 and every read became a hash lookup. | ||
|
|
||
| function vectorLength(a: unknown[]): number { | ||
| const m = /vector length: (\d+)/.exec(describeArray(a)); | ||
| if (!m) throw new Error(`no vector length in ${describeArray(a)}`); | ||
| return Number(m[1]); | ||
| } | ||
|
|
||
| function ints(n: number): number[] { | ||
| return Array.from({ length: n }, (_, i) => i); | ||
| } | ||
|
|
||
| const locks: [string, (a: unknown[]) => unknown[]][] = [ | ||
| ["Object.freeze", a => Object.freeze(a)], | ||
| ["Object.seal", a => Object.seal(a)], | ||
| ["Object.preventExtensions", a => Object.preventExtensions(a)], | ||
| ["non-writable length", a => Object.defineProperty(a, "length", { writable: false })], | ||
| ]; | ||
|
|
||
| const inputs: [string, () => unknown[], unknown[]][] = [ | ||
| ["int32", () => ints(16), ints(16)], | ||
| ["double", () => [0.5, 1.5, 2.5], [0.5, 1.5, 2.5]], | ||
| ["contiguous", () => ["a", null, 1], ["a", null, 1]], | ||
| ["holey", () => [1, , 3], [1, , 3]], | ||
| [ | ||
| "length beyond the vector", | ||
| () => { | ||
| const a = [1, 2, 3]; | ||
| a.length = 1000; | ||
| return a; | ||
| }, | ||
| Object.assign(new Array(1000), [1, 2, 3]), | ||
| ], | ||
| ]; | ||
|
|
||
| describe("frozen arrays keep their elements in the vector", () => { | ||
| describe.each(locks)("%s", (name, lock) => { | ||
| test.each(inputs)("%s", (_input, make, expected) => { | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| "use strict"; | ||
| const a = lock(make()); | ||
| expect(describeObject(a)).toContain("ArrayWithSlowPutArrayStorage"); | ||
| // The vector keeps its capacity, which holds at least every present element. | ||
| expect(vectorLength(a)).toBeGreaterThanOrEqual(Object.keys(expected).length); | ||
| expect(a.length).toBe(expected.length); | ||
| expect(a).toStrictEqual(expected); | ||
| expect(Object.keys(a)).toEqual(Object.keys(expected)); | ||
| expect(Object.isExtensible(a)).toBe(name === "non-writable length"); | ||
| expect(Object.getOwnPropertyDescriptor(a, "length")!.writable).toBe( | ||
| name === "Object.seal" || name === "Object.preventExtensions", | ||
| ); | ||
| // No lock lets the array grow: either the object is non-extensible or its length is read-only. | ||
| expect(() => { | ||
| a[a.length] = 1; | ||
| }).toThrow(TypeError); | ||
| expect(() => a.push(1)).toThrow(TypeError); | ||
| expect(a.length).toBe(expected.length); | ||
| // The structure answers isFrozen / isSealed only for the lock that earned it. | ||
| expect(Object.isFrozen(a)).toBe(name === "Object.freeze"); | ||
| expect(Object.isSealed(a)).toBe(name === "Object.freeze" || name === "Object.seal"); | ||
| }); | ||
|
|
||
| test("an empty array", () => { | ||
| "use strict"; | ||
| const a = lock(new Array()) as unknown[]; | ||
| expect(Object.isSealed(a)).toBe(name !== "non-writable length"); | ||
| expect(Object.isFrozen(a)).toBe(name === "Object.freeze"); | ||
| expect(() => a.push(1)).toThrow(TypeError); | ||
|
claude[bot] marked this conversation as resolved.
|
||
| expect(() => { | ||
| a[0] = 1; | ||
| }).toThrow(TypeError); | ||
| expect(Object.hasOwn(a, 0)).toBe(false); | ||
| expect(a.length).toBe(0); | ||
| if (name === "Object.freeze" || name === "non-writable length") { | ||
| expect(() => { | ||
| a.length = 1; | ||
| }).toThrow(TypeError); | ||
| expect(a.length).toBe(0); | ||
| } else { | ||
| a.length = 1; | ||
| expect(a.length).toBe(1); | ||
| expect(Object.hasOwn(a, 0)).toBe(false); | ||
| } | ||
| }); | ||
|
|
||
| test("an array that already owns a sparse map entry", () => { | ||
| "use strict"; | ||
| const b = [1, 2, 3]; | ||
| b[100000] = 4; | ||
| lock(b); | ||
| expect(b[0]).toBe(1); | ||
| expect(b[100000]).toBe(4); | ||
| expect(b.length).toBe(100001); | ||
| // A hole between the vector and the map entry cannot be filled on a non-extensible array. | ||
| if (Object.isExtensible(b)) { | ||
| b[50000] = 5; | ||
| expect(b[50000]).toBe(5); | ||
| } else { | ||
| expect(() => { | ||
| b[50000] = 5; | ||
| }).toThrow(TypeError); | ||
| expect(Object.hasOwn(b, 50000)).toBe(false); | ||
| } | ||
| expect(() => { | ||
| b[200000] = 5; | ||
| }).toThrow(TypeError); | ||
| expect(b.length).toBe(100001); | ||
| }); | ||
| }); | ||
|
|
||
| test("double and contiguous arrays", () => { | ||
| const d = Object.freeze([0.5, 1.5, 2.5]); | ||
| expect(vectorLength(d)).toBeGreaterThanOrEqual(3); | ||
| expect(d[0] + d[1] + d[2]).toBe(4.5); | ||
| const c = Object.freeze(["a", {}, 1]); | ||
| expect(vectorLength(c)).toBeGreaterThanOrEqual(3); | ||
| expect(c[0]).toBe("a"); | ||
| }); | ||
|
|
||
| test("frozen semantics hold on the vector", () => { | ||
| "use strict"; | ||
| const a = Object.freeze(ints(4)); | ||
| expect(() => { | ||
| a[0] = 9; | ||
| }).toThrow(TypeError); | ||
| expect(() => { | ||
| a[4] = 9; | ||
| }).toThrow(TypeError); | ||
| expect(() => a.push(9)).toThrow(TypeError); | ||
| expect(() => a.pop()).toThrow(TypeError); | ||
| expect(() => { | ||
| a.length = 0; | ||
| }).toThrow(TypeError); | ||
| expect(() => { | ||
| delete a[0]; | ||
| }).toThrow(TypeError); | ||
| expect(Object.getOwnPropertyDescriptor(a, 0)).toEqual({ | ||
| value: 0, | ||
| writable: false, | ||
| enumerable: true, | ||
| configurable: false, | ||
| }); | ||
| expect(Object.isFrozen(a)).toBe(true); | ||
|
robobun marked this conversation as resolved.
|
||
| expect([...a]).toEqual([0, 1, 2, 3]); | ||
| expect(vectorLength(a)).toBeGreaterThanOrEqual(4); | ||
| }); | ||
|
|
||
| test("sealed semantics hold on the vector", () => { | ||
| "use strict"; | ||
| const a = Object.seal(ints(4)); | ||
| a[0] = 9; | ||
| expect(a[0]).toBe(9); | ||
| expect(() => { | ||
| delete a[0]; | ||
| }).toThrow(TypeError); | ||
| expect(() => a.pop()).toThrow(TypeError); | ||
| expect(() => { | ||
| a.length = 1; | ||
| }).toThrow(TypeError); | ||
| expect(a.length).toBe(4); | ||
| expect(() => a.push(9)).toThrow(TypeError); | ||
| expect(Object.getOwnPropertyDescriptor(a, 0)).toEqual({ | ||
| value: 9, | ||
| writable: true, | ||
| enumerable: true, | ||
| configurable: false, | ||
| }); | ||
| expect(Object.isSealed(a)).toBe(true); | ||
| expect(Object.isFrozen(a)).toBe(false); | ||
| expect(vectorLength(a)).toBeGreaterThanOrEqual(4); | ||
| }); | ||
|
|
||
| test("a frozen array on the prototype chain rejects inherited writes", () => { | ||
| "use strict"; | ||
| const proto = Object.freeze(ints(3)); | ||
| const child = Object.create(proto); | ||
| expect(() => { | ||
| child[0] = 9; | ||
| }).toThrow(TypeError); | ||
| expect(child[0]).toBe(0); | ||
| expect(Object.hasOwn(child, 0)).toBe(false); | ||
|
|
||
| // Frozen after it became a prototype, with a plain child and an array child with a hole. | ||
| const late = ints(3); | ||
| const plainChild = Object.create(late); | ||
| const arrayChild = [9, , 9]; | ||
| Object.setPrototypeOf(arrayChild, late); | ||
| Object.freeze(late); | ||
| expect(() => { | ||
| plainChild[1] = 9; | ||
| }).toThrow(TypeError); | ||
| expect(Object.hasOwn(plainChild, 1)).toBe(false); | ||
| expect(() => { | ||
| arrayChild[1] = 9; | ||
| }).toThrow(TypeError); | ||
| expect(Object.hasOwn(arrayChild, 1)).toBe(false); | ||
| expect(arrayChild[1]).toBe(1); | ||
|
|
||
| // Object.setPrototypeOf to an array frozen earlier. | ||
| const grown = [9, , 9]; | ||
| Object.setPrototypeOf(grown, proto); | ||
| expect(() => { | ||
| grown[1] = 9; | ||
| }).toThrow(TypeError); | ||
| expect(grown[1]).toBe(1); | ||
| grown[3] = 3; | ||
| expect(grown.length).toBe(4); | ||
|
|
||
| // A sealed prototype does not stop the child from shadowing. | ||
| const sealed = Object.seal([7, 8, 9]); | ||
| const shadow = Object.create(sealed); | ||
| shadow[0] = 77; | ||
| expect(shadow[0]).toBe(77); | ||
| expect(sealed[0]).toBe(7); | ||
| }); | ||
|
|
||
| test("a store site warmed on a writable non-extensible array rejects a frozen one", () => { | ||
| function store(a: number[], i: number, v: number) { | ||
| "use strict"; | ||
|
robobun marked this conversation as resolved.
|
||
| a[i] = v; | ||
| } | ||
| const writable = Object.preventExtensions(ints(8)); | ||
| for (let i = 0; i < 100_000; i++) store(writable, i & 7, i); | ||
| const frozen = Object.freeze(ints(8)); | ||
| for (let i = 0; i < 1000; i++) { | ||
| expect(() => store(frozen, i & 7, -1)).toThrow(TypeError); | ||
| store(writable, i & 7, -1); | ||
| } | ||
| expect(frozen).toEqual([0, 1, 2, 3, 4, 5, 6, 7]); | ||
| expect(writable).toEqual([-1, -1, -1, -1, -1, -1, -1, -1]); | ||
|
robobun marked this conversation as resolved.
|
||
| }); | ||
|
|
||
| test("defineProperty on one element moves the array to the sparse map with exact descriptors", () => { | ||
| const a = Object.seal(ints(3)); | ||
| Object.defineProperty(a, 0, { writable: false }); | ||
| expect(Object.getOwnPropertyDescriptor(a, 0)).toEqual({ | ||
| value: 0, | ||
| writable: false, | ||
| enumerable: true, | ||
| configurable: false, | ||
| }); | ||
| expect(Object.getOwnPropertyDescriptor(a, 1)).toEqual({ | ||
| value: 1, | ||
| writable: true, | ||
| enumerable: true, | ||
| configurable: false, | ||
| }); | ||
| expect(vectorLength(a)).toBe(0); | ||
| expect(() => Object.defineProperty(a, 1, { configurable: true })).toThrow(TypeError); | ||
| expect(Object.isSealed(a)).toBe(true); | ||
| expect(Object.isFrozen(a)).toBe(false); | ||
| // Frozen through the element-wise route: the generic walk, not the structure, answers. | ||
| Object.defineProperty(a, 1, { writable: false }); | ||
| Object.defineProperty(a, 2, { writable: false }); | ||
| expect(Object.isFrozen(a)).toBe(false); | ||
| Object.defineProperty(a, "length", { writable: false }); | ||
| expect(Object.isFrozen(a)).toBe(true); | ||
| const b = Object.preventExtensions([1, 2]); | ||
| expect(Object.isSealed(b)).toBe(false); | ||
| Object.defineProperty(b, 0, { configurable: false }); | ||
| Object.defineProperty(b, 1, { configurable: false }); | ||
| expect(Object.isSealed(b)).toBe(true); | ||
| expect(Object.isFrozen(b)).toBe(false); | ||
| }); | ||
|
|
||
| test.concurrent("freezing one literal leaves the next literal from the same site writable", async () => { | ||
| // A separate process: another test file can put this realm in "having a bad time" mode, and | ||
| // then every array is SlowPutArrayStorage. | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| `const { describe } = require("bun:jsc"); | ||
| function make() { return [1, 2, 3, 4]; } | ||
| function build(n) { const a = []; for (let i = 0; i < n; i++) a.push(i); return a; } | ||
| let slow = 0; | ||
| for (let i = 0; i < 200; i++) { | ||
| const a = Object.freeze(make()); | ||
| const b = make(); | ||
| b[0] = 9; | ||
| b.push(5); | ||
| if (b.join() !== "9,2,3,4,5" || a.join() !== "1,2,3,4") throw new Error("bad values"); | ||
| if (describe(b).includes("SlowPutArrayStorage")) slow++; | ||
| Object.freeze(build(8)); | ||
| if (describe(build(8)).includes("SlowPutArrayStorage")) slow++; | ||
|
robobun marked this conversation as resolved.
|
||
| } | ||
| console.log(slow);`, | ||
| ], | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(stdout).toBe("0\n"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test.concurrent("freezing Array.prototype keeps it blank", async () => { | ||
| // A separate process, so the rest of the test run keeps a writable Array.prototype. | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| `const { describe } = require("bun:jsc"); | ||
| Object.freeze(Array.prototype); | ||
| let threw = false; | ||
| try { Array.prototype.push.call(Array.prototype, 1); } catch (e) { threw = e instanceof TypeError; } | ||
| // Reflect.set reports the rejection whatever the strictness of this script is. | ||
| const rejected = [Reflect.set(Array.prototype, 0, 1), Reflect.set(Array.prototype, "length", 1)].map(ok => !ok); | ||
| console.log(JSON.stringify({ threw, rejected, own: Object.hasOwn(Array.prototype, 0), length: Array.prototype.length, frozen: Object.isFrozen(Array.prototype), blank: describe(Array.prototype).includes("ArrayClass") }));`, | ||
| ], | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(JSON.parse(stdout)).toEqual({ | ||
| threw: true, | ||
| rejected: [true, true], | ||
| own: false, | ||
| length: 0, | ||
| frozen: true, | ||
| blank: true, | ||
| }); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Every build after the WebKit PR merges or closes will fail to download JavaScriptCore, because the pin is to a preview release GitHub deletes. scripts/build/deps/webkit.ts:6 sets WEBKIT_VERSION to
autobuild-preview-pr-752-98c438b4, anautobuild-preview-pr-*tag that only exists while oven-sh/WebKit#752 is open. Fix: before merge, pin WEBKIT_VERSION to the merged main commit'sautobuild-<sha>(or the 40-hex sha) and confirm prebuilt tarballs exist for every platform x flavor. The description calls the preview pin temporary; that note does not protect main once the engine PR lands.Why this was flagged
The pin at scripts/build/deps/webkit.ts:6 is
autobuild-preview-pr-752-98c438b4. scripts/build/download.ts:320-323 documents that GitHub deletes theautobuild-preview-pr-*release when the WebKit PR merges or closes, so every prebuilt download 404s at once; prebuiltDownloadError at download.ts:328-336 then fails the build with "WebKit preview release is gone". .claude/commands/upgrade-webkit.md:34 requires the bump to the merge commit'sautobuild-<sha>before the bun PR merges. On the base branch the pin is the 40-hex sha fb1167ebf2cb9edc1f6771a2c11771b024693ae0 whose release is permanent, so CI and localbun bdkeep working. After this merges, the moment oven-sh/WebKit#752 lands or closes, every CI build and every developer build on main breaks until someone edits the pin. The PR description says the pin moves to the merge commit once the WebKit PR lands, but nothing in this checkout enforces that ordering.Verification: The PR description calls the pin temporary, but the diff as it stands still merges it. scripts/build/deps/webkit.ts:6 sets WEBKIT_VERSION to
autobuild-preview-pr-752-98c438b4, which prebuiltUrl() at webkit.ts:68-76 uses verbatim as the release tag. On the base branch the pin is a permanentautobuild-<sha>release, so once oven-sh/WebKit#752 merges or closes every prebuilt bun build fails to download JavaScriptCore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional for now: the pin follows the preview of oven-sh/WebKit#752 until that PR merges. Before this PR merges the pin moves to the merge commit's autobuild release. Leaving this thread open as the reminder.