You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[JSC] Hold the value and the key of a super property store in a RefPtr across ensureThis() - #1016
In an arrow function of a derived constructor, super[key] = 21 can evaluate to the scope object of the constructor, and the setter gets that object. for (super[key] of list) there throws TypeError: No default value.
AssignBracketNode::emitBytecode (NodesCodegen.cpp:4309) and ForOfNode::emitBytecode (:4944) hold the value register and the key register by raw pointer across ensureThis(). Where ensureThis() reads this again, its temporary for the scope is that same register.
Script reads and writes the parameters and vars of the constructor through the object. An accessor defined on it aborts at RELEASE_ASSERT(!(attributes & PropertyAttribute::Accessor)) (JSLexicalEnvironment.cpp:103). No memory is corrupted.
Fix
Both registers are RefPtr<RegisterID>, like the others that live across ensureThis().
Verified: JSTests/stress/super-property-assignment-value-in-derived-constructor-arrow-function.js (new, fails on the base with and without the JIT).
The bytecode generator counts references to a temporary register. A register that only a raw RegisterID* names is free for the next newTemporary().
ensureThis() emits the check that this has a value. In an arrow function that has eval code or an arrow function with super(), and in the body of an async arrow function, it reads this from the scope first.
Considered an ensureThis() that takes no temporary: the two callers would still hold free registers.
Downsides
None found. Checked: 20,790 of 20,790 code blocks of 545 JSTests/stress files compile to the same bytecode and register count.
Notes
Route.
The two functions are upstream's code. WebKit/WebKit main (3f5092b, 2026-11-08) has the same lines: RegisterID* forwardResult at NodesCodegen.cpp:4155 and RegisterID* subscript at :4772. The patch has no fork guard.
@sosukesuzuki: two questions. Will you file this upstream? I cannot file the bugs.webkit.org entry or the WebKit/WebKit PR. And does a scope object that reaches script go through WebKit's security component? The forms below are public on this branch.
The value must be a constant: generator.move(generator.tempDestination(result.get()), result.get()) then makes a temporary that nothing refers to. The result of the assignment must be used.
Not affected: the body of the constructor. Its scope is in a register of its own, so the read of this takes no temporary there.
The scope object is a JSLexicalEnvironment. Object.prototype.toString.call() of it is [object Undefined]. Reflect.ownKeys() lists arguments, the parameters and the vars of the constructor. A store to one of them changes the variable that the closures of the constructor read. A store to a new name adds a property.
Object.defineProperty(scopeObject, "a", { get() {} }) succeeds. The next read of scopeObject.a aborts at the RELEASE_ASSERT.
Self-review. The review was of a larger change that had these two hunks. It asked for three things here, and all are done: a PR of their own that goes first, a test of their own with the forms that need no await, and a description that says what script gets and that the process aborts.
Test runs. Debug build with ASAN. The base fails the new test at its first assertion in the default configuration, with --useJIT=0 and with --useBytecodeOptimizer=1. This branch passes in the 17 modes of run-jsc-stress-tests, alone and together with #1017 and #1018.
… that a read of `this` cannot take
AssignBracketNode::emitBytecode() and ForOfNode::emitBytecode() hold a register by a raw pointer across
BytecodeGenerator::ensureThis(): the value of `super[key] = value`, and the key of `for (super[key] of ...)`.
Nothing else refers to that register, so the generator can give it out again. Where ensureThis() reads `this`
again from the scope, it takes a temporary for the scope and gets that register. The store then uses the scope
object as the value, or as the key.
ensureThis() reads `this` again in an arrow function of a derived constructor that has eval code or an arrow
function with super(), and in the body of an async arrow function of a derived constructor. There:
const a = key => { eval(""); return super[key] = 21; }; // a("x") is the scope object
const b = key => { (() => { super(); }); return super[key] = 21; }; // the same
const c = async key => { await 0; return super[key] = 21; }; // resolves with the scope object
const d = () => { eval(""); for (super[name()] of [1]); }; // TypeError: No default value
The setter of the property gets the scope object too. It is the JSLexicalEnvironment of the constructor. Script
reads and writes the parameters and the `var` variables of the constructor through it, and can add properties.
An accessor property that script defines on it reaches RELEASE_ASSERT(!(attributes & PropertyAttribute::Accessor))
in JSLexicalEnvironment::getOwnPropertySlot() at the next read: the process aborts. No memory is corrupted.
Both registers are RefPtr<RegisterID> now, as the other registers that live across ensureThis() are.
Test: JSTests/stress/super-property-assignment-value-in-derived-constructor-arrow-function.js
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the compound-store path (super.x += v, ReadModifyDotNode at NodesCodegen.cpp:4236-4249) for the same free-register hazard — it caches thisValue during the get at :1258, so the put at :1314 never calls ensureThis() again while the raw updatedValue is live. The null-dst return at :4330 follows the usual emitNode contract (caller takes the result into a RefPtr immediately), so it is not a new hazard. The new stress test was not run as part of this review.
Extended reasoning...
The change converts two raw RegisterID* locals in AssignBracketNode::emitBytecode and ForOfNode::emitBytecode to RefPtr so the value and subscript registers stay allocated while ensureThis() may call newTemporary() via emitResolveScope(nullptr, ...), and adds a stress test plus a row in JSTests/BUN-TEST-DIFFERENCES.md. It touches the bytecode generator of the JS engine, where a reused register exposes the arrow-function scope object to script; no auth, injection or data-exposure surface is involved. The paths are covered by CODEOWNERS (/Source/JavaScriptCore and /JSTests), and one finding was reported inline, so this is not approved; the note records the adjacent paths examined and ruled out.
No actionable comments were generated in the recent review.
Walkthrough
The bytecode emitters now retain bracket-assignment registers with RefPtr. A new stress test checks super property assignments in derived-constructor arrow functions, and the differences document records the test.
AssignBracketNode::emitBytecode and ForOfNode::emitBytecode now manage the relevant registers with RefPtr. Existing property writes and the returned assignment value remain unchanged.
The stress test checks assignment results and setter values across synchronous and asynchronous arrow-function cases. The differences document adds a row describing the test and its expected behavior.
Severity of issue fixed: Low
Merge Risk: Minimal · up to 6ce25
No concrete issue remains that would prevent merging after normal checks.
Pre-merge checks: 3 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Description check
The description is detailed and follows the substance of the WebKit template in part. It states the problem, the fix, and the verification. It names the affected functions and lines, and it gives a te…
Rewrite the description in the WebKit commit-message format from the template. Include: (1) a bug title line; (2) the Bugzilla URL in the form https://bugs.webkit.org/show_bug.cgi?id=#####, or a clear statement that the change needs a bug f…
Passed checks: 3 passed
Check name
Status
Explanation
Title check
The title names the main change: both the value register and the key register of a super property store are now held in a RefPtr across ensureThis(). It matches the raw summary and the objectives. It …
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the main change: both the value register and the key register of a super property store are now held in a RefPtr across ensureThis(). It matches the raw summary and the objectives. It is specific to the JSC bytecode fix and does not add file lists or vague terms. The title is 90 characters, which is long, but it stays readable.
Full details: Description check
Explanation
The description is detailed and follows the substance of the WebKit template in part. It states the problem, the fix, and the verification. It names the affected functions and lines, and it gives a test file and test results. It also explains the background and the downsides. It does not follow the required WebKit template. It has no bug title line and no Bugzilla link (https://bugs.webkit.org/show_bug.cgi?id=#####). It has no 'Reviewed by' line. Its explanation section does not follow the template's commit-message format. It lists no changed files in the '* path/changed.ext:' format. The author states that a Bugzilla entry does not exist yet and that the author cannot file it. The template requires a bug association for every change. The description therefore misses several required elements, but it is not vague or off-topic.
Resolution
Rewrite the description in the WebKit commit-message format from the template. Include: (1) a bug title line; (2) the Bugzilla URL in the form https://bugs.webkit.org/show_bug.cgi?id=#####, or a clear statement that the change needs a bug filed and the bug ID to be added once it exists; (3) a 'Reviewed by' line, which the author fills in after review; (4) the explanation of why the change fixes the bug, written in plain prose and kept short; and (5) a changed-files list in the '* path/changed.ext:' format with the affected function names, for example '* JSTests/stress/super-property-assignment-value-in-derived-constructor-arrow-function.js:', '* Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:' with '(JSC::AssignBracketNode::emitBytecode):' and '(JSC::ForOfNode::emitBytecode):', and '* JSTests/BUN-TEST-DIFFERENCES.md:'. Also state that the PR must be mentioned on Bugzilla and that the PR needs component and version labels and an assignee, as the template requires.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 path_filters to narrow the review scope.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
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
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
For WebKit/WebKit too: see Route in Notes.
Problem
super[key] = 21can evaluate to the scope object of the constructor, and the setter gets that object.for (super[key] of list)there throwsTypeError: No default value.AssignBracketNode::emitBytecode(NodesCodegen.cpp:4309) andForOfNode::emitBytecode(:4944) hold the value register and the key register by raw pointer acrossensureThis(). WhereensureThis()readsthisagain, its temporary for the scope is that same register.vars of the constructor through the object. An accessor defined on it aborts atRELEASE_ASSERT(!(attributes & PropertyAttribute::Accessor))(JSLexicalEnvironment.cpp:103). No memory is corrupted.Fix
RefPtr<RegisterID>, like the others that live acrossensureThis().JSTests/stress/super-property-assignment-value-in-derived-constructor-arrow-function.js(new, fails on the base with and without the JIT).Background
RegisterID*names is free for the nextnewTemporary().ensureThis()emits the check thatthishas a value. In an arrow function that has eval code or an arrow function withsuper(), and in the body of an async arrow function, it readsthisfrom the scope first.ensureThis()that takes no temporary: the two callers would still hold free registers.Downsides
JSTests/stressfiles compile to the same bytecode and register count.Notes
Route.
RegisterID* forwardResultatNodesCodegen.cpp:4155andRegisterID* subscriptat:4772. The patch has no fork guard.AssignBracketNode::emitBytecode, which emits the store form of [JSC] Super property should checkthisbinding before evaluating the property expression WebKit/WebKit#76967. Its upstream form can go in or beside that PR.maintoday. If upstream takes it in another form, a sync replaces these lines and the test stays.WEBKIT_VERSIONbump of its own. [JSC] Arrow functions and eval code of a derived constructor readthisat each use when other code of the constructor can call super() #1018 and [JSC] Predict the empty value for an unprofiled load ofthisof a derived constructor from its scope #1017 are independent of it.The forms.
Basehasset x(value). Each arrow function is made in the constructor ofclass Derived extends Base, aftersuper(). The base is 5a61071.key => { eval(""); return super[key] = 21; }2121key => { (() => { super(); }); return super[key] = 21; }2121async key => { await 0; return super[key] = 21; }2121() => { eval(""); for (super[name()] of [1]); }TypeError: No default valuegenerator.move(generator.tempDestination(result.get()), result.get())then makes a temporary that nothing refers to. The result of the assignment must be used.thistakes no temporary there.JSLexicalEnvironment.Object.prototype.toString.call()of it is[object Undefined].Reflect.ownKeys()listsarguments, the parameters and thevars of the constructor. A store to one of them changes the variable that the closures of the constructor read. A store to a new name adds a property.Object.defineProperty(scopeObject, "a", { get() {} })succeeds. The next read ofscopeObject.aaborts at theRELEASE_ASSERT.Self-review. The review was of a larger change that had these two hunks. It asked for three things here, and all are done: a PR of their own that goes first, a test of their own with the forms that need no
await, and a description that says what script gets and that the process aborts.Test runs. Debug build with ASAN. The base fails the new test at its first assertion in the default configuration, with
--useJIT=0and with--useBytecodeOptimizer=1. This branch passes in the 17 modes ofrun-jsc-stress-tests, alone and together with #1017 and #1018.