Repository navigation
[JSC] Freezing built-in prototypes should not permanently disable fast paths #622
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
Jarred-Sumner
wants to merge
1
commit into
main
Choose a base branch
from
jarred/cheaper-freeze
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
Changes from all commits
Commits
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
90 changes: 90 additions & 0 deletions
90
JSTests/stress/define-property-same-value-keeps-adaptive-watchpoints.js
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,90 @@ | ||
| function assert(cond, msg) { | ||
| if (!cond) | ||
| throw new Error("FAIL: " + msg); | ||
| } | ||
|
|
||
| function warm(f, n = 1e4) { | ||
| let r; | ||
| for (let i = 0; i < n; i++) | ||
| r = f(i); | ||
| return r; | ||
| } | ||
|
|
||
| { | ||
| const proto = { method() { return 1; } }; | ||
| const o = Object.create(proto); | ||
| const read = () => o.method(); | ||
| warm(read); | ||
| Object.defineProperty(proto, "method", { writable: false }); | ||
| assert(warm(read) === 1, "same-value attribute change keeps value"); | ||
| Object.defineProperty(proto, "method", { value: () => 2 }); | ||
| assert(warm(read) === 2, "configurable read-only property redefined with a new value is observed"); | ||
| } | ||
|
|
||
| { | ||
| const origExec = RegExp.prototype.exec; | ||
| const run = () => "a-b".replace(/-/g, "+"); | ||
| warm(run, 1e3); | ||
| Object.defineProperty(RegExp.prototype, "exec", { writable: false }); | ||
| assert(run() === "a+b", "replace still works after exec made read-only"); | ||
| let called = 0; | ||
| Object.defineProperty(RegExp.prototype, "exec", { value: function (s) { called++; return origExec.call(this, s); } }); | ||
| assert(run() === "a+b" && called > 0, "replaced exec is called by String.prototype.replace"); | ||
| Object.defineProperty(RegExp.prototype, "exec", { value: origExec }); | ||
| } | ||
|
|
||
| { | ||
| const o = {}; | ||
| const g1 = () => 1; | ||
| const g2 = () => 2; | ||
| Object.defineProperty(o, "x", { get: g1, configurable: true }); | ||
| const read = () => o.x; | ||
| warm(read); | ||
| Object.defineProperty(o, "x", { enumerable: true }); | ||
| assert(warm(read) === 1, "accessor attribute change keeps getter"); | ||
| const desc = Object.getOwnPropertyDescriptor(o, "x"); | ||
| assert(desc.get === g1 && desc.enumerable, "descriptor updated"); | ||
| Object.defineProperty(o, "x", { get: g2 }); | ||
| assert(warm(read) === 2, "new getter observed"); | ||
| Object.defineProperty(o, "x", { set(v) { this._v = v; } }); | ||
| assert(o.x === 2, "getter preserved when only setter changes"); | ||
| o.x = 5; | ||
| assert(o._v === 5, "new setter called"); | ||
| Object.defineProperty(o, "x", { get: undefined }); | ||
| assert(o.x === undefined, "getter cleared"); | ||
| } | ||
|
|
||
| { | ||
| class MyArray extends Array { } | ||
| const a = MyArray.from([1, 2, 3]); | ||
| Object.defineProperty(Array, Symbol.species, { configurable: false }); | ||
| assert(a.map(x => x) instanceof MyArray, "subclass species still honored"); | ||
| assert([1, 2].map(x => x).constructor === Array, "plain array species"); | ||
| } | ||
|
|
||
| { | ||
| Object.freeze(Object.prototype); | ||
| Object.freeze(Array.prototype); | ||
| Object.freeze(Function.prototype); | ||
| Object.freeze(RegExp.prototype); | ||
| Object.freeze(String.prototype); | ||
| Object.freeze(Promise.prototype); | ||
| Object.freeze(Map.prototype); | ||
| Object.freeze(Set.prototype); | ||
| assert(Object.isFrozen(Object.prototype) && Object.isFrozen(RegExp.prototype), "isFrozen"); | ||
| for (let i = 0; i < 1e3; i++) { | ||
| assert("a-b".replace(/-/g, "+") === "a+b", "replace after freeze"); | ||
| assert([..."abc"].join("") === "abc", "string spread after freeze"); | ||
| assert(String(new String("x")) === "x", "String(obj) after freeze"); | ||
| assert([1, 2] + "" === "1,2", "array join after freeze"); | ||
| assert([...new Set([1, 2])].length === 2 && new Map([[1, 2]]).get(1) === 2, "Map/Set after freeze"); | ||
| } | ||
| let threw = false; | ||
| try { | ||
| (() => { "use strict"; ({}).toString = 1; })(); | ||
| } catch { | ||
| threw = true; | ||
| } | ||
| assert(threw, "override mistake still throws"); | ||
| assert(Object.getOwnPropertyDescriptor(RegExp.prototype, "flags").configurable === false, "accessor frozen"); | ||
| } | ||
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,85 @@ | ||
| function assert(cond, msg) { | ||
| if (!cond) | ||
| throw new Error("FAIL: " + msg); | ||
| } | ||
|
|
||
| function throwsTypeError(f) { | ||
| try { | ||
| f(); | ||
| } catch (e) { | ||
| return e instanceof TypeError; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function strictSetLength(o, v) { "use strict"; o.length = v; } | ||
| function strictSetIndex(o, i, v) { "use strict"; o[i] = v; } | ||
|
|
||
| function readHoles(n) { | ||
| const holey = [1, , 3, , 5]; | ||
| let undefs = 0; | ||
| for (let i = 0; i < n; i++) { | ||
| if (holey[i % 5] === undefined) | ||
| undefs++; | ||
| } | ||
| return undefs; | ||
| } | ||
| assert(readHoles(1e4) === 4e3, "holes before freeze"); | ||
|
|
||
| const AP = Array.prototype; | ||
| Object.freeze(AP); | ||
|
|
||
| assert(Object.isFrozen(AP), "isFrozen"); | ||
| assert(!Object.isExtensible(AP), "not extensible"); | ||
| const lengthDesc = Object.getOwnPropertyDescriptor(AP, "length"); | ||
| assert(lengthDesc.value === 0 && !lengthDesc.writable && !lengthDesc.configurable && !lengthDesc.enumerable, "length descriptor"); | ||
| assert(Object.getOwnPropertyDescriptor(AP, "push").writable === false, "method read-only"); | ||
|
|
||
| assert(throwsTypeError(() => strictSetLength(AP, 1)), "strict length write throws"); | ||
| AP.length = 1; | ||
| assert(AP.length === 0, "sloppy length write ignored"); | ||
| assert(throwsTypeError(() => strictSetLength(AP, 0)), "strict same-value length write throws"); | ||
| assert(Reflect.set(AP, "length", 0) === false, "Reflect.set length"); | ||
| assert(Reflect.defineProperty(AP, "length", { value: 0 }) === true, "same-value define length"); | ||
| assert(Reflect.defineProperty(AP, "length", { value: 1 }) === false, "different-value define length"); | ||
|
|
||
| assert(throwsTypeError(() => strictSetIndex(AP, 0, 1)), "strict index write throws"); | ||
| AP[0] = 1; | ||
| assert(AP[0] === undefined && !Object.hasOwn(AP, 0), "sloppy index write ignored"); | ||
| assert(throwsTypeError(() => Object.defineProperty(AP, 0, { value: 1 })), "define index throws"); | ||
| assert(Reflect.defineProperty(AP, 3, { value: 1 }) === false, "Reflect.defineProperty index"); | ||
|
|
||
| assert(throwsTypeError(() => AP.push.call(AP, 1)), "push throws"); | ||
| assert(throwsTypeError(() => AP.push.call(AP)), "push with no args throws"); | ||
| assert(throwsTypeError(() => AP.pop.call(AP)), "pop throws"); | ||
| assert(throwsTypeError(() => AP.shift.call(AP)), "shift throws"); | ||
| assert(throwsTypeError(() => AP.unshift.call(AP, 1)), "unshift throws"); | ||
| assert(throwsTypeError(() => AP.unshift.call(AP)), "unshift no args throws"); | ||
| assert(throwsTypeError(() => AP.splice.call(AP, 0, 0, 1)), "splice insert throws"); | ||
| assert(AP.length === 0 && Object.getOwnPropertyNames(AP).every(k => isNaN(+k) || k === ""), "no indexed props leaked"); | ||
|
|
||
| for (let i = 0; i < 1e4; i++) { | ||
| assert(throwsTypeError(() => AP.push.call(AP, i)), "push throws (warm)"); | ||
| assert(throwsTypeError(() => AP.pop.call(AP)), "pop throws (warm)"); | ||
| assert(throwsTypeError(() => strictSetLength(AP, i)), "length write throws (warm)"); | ||
| } | ||
|
|
||
| assert(readHoles(1e4) === 4e3, "holes after freeze"); | ||
| assert([1, , 3].includes(undefined) && [, 2].indexOf(undefined) === -1, "includes/indexOf holes"); | ||
| assert([...[1, , 3]].length === 3 && [...[1, , 3]][1] === undefined, "spread holes"); | ||
| assert([1, , 3].slice(0)[1] === undefined && !(1 in [1, , 3].slice(0)), "slice holes"); | ||
|
|
||
| const frozenEmpty = Object.freeze([]); | ||
| assert(throwsTypeError(() => frozenEmpty.push(1)) && throwsTypeError(() => frozenEmpty.pop()), "frozen empty literal"); | ||
| const frozenNewArray = Object.freeze(new Array()); | ||
| assert(throwsTypeError(() => frozenNewArray.push(1)) && throwsTypeError(() => frozenNewArray.pop()), "frozen new Array()"); | ||
| assert(throwsTypeError(() => strictSetLength(frozenNewArray, 0)), "frozen new Array() length"); | ||
| assert(Object.isFrozen(frozenNewArray) && frozenNewArray.length === 0, "frozen new Array() state"); | ||
|
|
||
| const sealed = Object.seal(new Array()); | ||
| assert(throwsTypeError(() => sealed.push(1)), "sealed empty push throws"); | ||
| sealed.length = 5; | ||
| assert(sealed.length === 5 && !(0 in sealed), "sealed empty length writable"); | ||
|
|
||
| if (typeof $vm !== "undefined") | ||
| assert($vm.indexingMode(AP) === "ArrayClass", "frozen Array.prototype keeps blank indexing after rejected writes: " + $vm.indexingMode(AP)); |
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,63 @@ | ||
| function assert(cond, msg) { | ||
| if (!cond) | ||
| throw new Error("FAIL: " + msg); | ||
| } | ||
|
|
||
| function throwsTypeError(f) { | ||
| try { | ||
| f(); | ||
| } catch (e) { | ||
| return e instanceof TypeError; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| Object.freeze(Object.prototype); | ||
|
|
||
| function assignOne(t, s) { return Object.assign(t, s); } | ||
| function assignTwo(t, s1, s2) { return Object.assign(t, s1, s2); } | ||
|
|
||
| for (let i = 0; i < 1e4; i++) { | ||
| const r = assignOne({}, { a: i, b: 2 }); | ||
| assert(r.a === i && r.b === 2, "plain assign"); | ||
| } | ||
|
|
||
| for (let i = 0; i < 1e4; i++) { | ||
| const t = {}; | ||
| assert(throwsTypeError(() => assignOne(t, { a: 1, toString: 2, b: 3 })), "colliding key throws"); | ||
| assert(t.a === 1 && !Object.hasOwn(t, "toString") && !Object.hasOwn(t, "b"), "keys before collision assigned, after not"); | ||
| } | ||
|
|
||
| for (let i = 0; i < 1e4; i++) { | ||
| const t = {}; | ||
| assert(throwsTypeError(() => assignTwo(t, { a: 1 }, { constructor: 2 })), "multi-source collision throws"); | ||
| assert(t.a === 1 && !Object.hasOwn(t, "constructor"), "first source assigned"); | ||
| const r = assignTwo({}, { a: 1 }, { b: 2 }); | ||
| assert(r.a === 1 && r.b === 2, "multi-source plain"); | ||
| } | ||
|
|
||
| { | ||
| let setterCalls = 0; | ||
| const src = { x: 1, y: 2 }; | ||
| const proto = Object.freeze(Object.create(Object.prototype, { | ||
| x: { set(v) { setterCalls++; this._x = v; src.y = 99; }, get() { return this._x; } }, | ||
| })); | ||
| for (let i = 0; i < 1e4; i++) { | ||
| src.y = 2; | ||
| const t = Object.create(proto); | ||
| assignOne(t, src); | ||
| assert(t._x === 1 && t.y === 99 && !Object.hasOwn(t, "x"), "setter invoked and later key re-read"); | ||
| } | ||
| assert(setterCalls === 1e4, "setter call count"); | ||
| } | ||
|
|
||
| for (let i = 0; i < 1e4; i++) { | ||
| const r = assignOne({}, JSON.parse('{"__proto__": {"polluted": 1}, "k": 1}')); | ||
| assert(r.k === 1, "json source with __proto__ key"); | ||
| assert(!Object.hasOwn(r, "__proto__") && r.polluted === 1, "__proto__ key goes through the Object.prototype setter"); | ||
| } | ||
|
|
||
| { | ||
| const r = Object.assign({}, { a: 1 }, [7, 8]); | ||
| assert(r.a === 1 && r[0] === 7 && r[1] === 8, "indexed source"); | ||
| } |
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
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
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
Oops, something went wrong.
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.
🟡 nit (optional): New stress tests hard-code iteration counts (1e4/1e3) instead of using
testLoopCount, which JSTests/README.md (imported by JSTests/CLAUDE.md) lists as a required rule so tests tier up only in configurations where it matters and stay under 200ms elsewhere. sweep:\b1e[34]\bin the three added JSTests/stress files. Fix: drive warm-up loops withtestLoopCount(e.g.function warm(f, n = testLoopCount)here;for (let i = 0; i < testLoopCount; i++)in freeze-array-prototype.js:61 and object-assign-frozen-object-prototype.js:21/26/32/42/51). [also at: JSTests/stress/object-assign-frozen-object-prototype.js:20 - nit: new stress tests hard-code1e4/1e3iteration counts instead oftestLoopCount; JSTests/README.md (imported by…]Extended reasoning...
JSTests/CLAUDE.md imports JSTests/README.md, whose rule 2 states new tests are required to use
testLoopCount/wasmTestLoopCountso the harness can scale iterations per configuration (eager tier-up vs. no-JIT vs. GC-heavy). All three new tests instead hard-code1e4(and1e3) loops: define-property-same-value-keeps-adaptive-watchpoints.js:6/75, freeze-array-prototype.js:27/61/67, object-assign-frozen-object-prototype.js:21/26/32/42/51. In no-JIT or GC-stress configurations these loops run at fixed cost with no tier-up benefit, risking the 200ms budget, and in eager configurations they may over-iterate. Base branch has no such files; the diff introduces the violation.Verification: nit — JSTests/README.md:20 (imported by JSTests/CLAUDE.md:1 via
@ README.md) states as a hard rule for new tests: "UsetestLoopCountorwasmTestLoopCountto control how many iterations a test runs. ThejscCLI sets these based on the configuration of the test, so tests iterate enough to tier up where that matters and exit early where it doesn't." All three new tests hard-code counts…