| Summary: | The rhs in `ReadModifyResolveNode` should be evaluated before throwing an exception if the lhs is read-only | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||
| Component: | JavaScriptCore | Assignee: | Devin Rousso <hi> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ews-watchlist, hi, joepeck, keith_miller, mark.lam, msaboff, rmorisset, ross.kirsling, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Local Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 396038 [details]
Patch
Comment on attachment 396038 [details]
Patch
Does this cause a test262 test to change, or maybe not one we have imported?
Comment on attachment 396038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396038&action=review > JSTests/stress/const-read-modify-assignment-eval-rhs-before-exception.js:24 > + try { > + testConstReadModifyAssignmentRHSEvalBeforeException += (() => ++count)(); > + } catch { } I'm a bit worried about the fact that this test doesn't fail before your patch. I spent a good while playing and I'm honestly not sure how to hit the `!var.local() && var.isReadOnly()` path in ReadModifyResolveNode (or AssignResolveNode). (Tried top-level const, non-configurable non-writable global var, freezing the global object and then trying to set a global var in strict mode...these all lack `var.local()`, but none satisfy `var.isReadOnly()`.) Alright, so here is a test that hits the non-local read-only path, fails before your patch, and succeeds with it:
(function () {
let count = 0;
const x = 42;
function captureX() { return x; }
try {
x -= (() => count++)();
} catch {
if (count === 1)
return;
}
throw new Error('failed!');
})();
I...actually don't understand why. I just adapted it from here:
https://github.com/WebKit/webkit/blob/master/JSTests/stress/const-semantics.js#L125-L129
(In reply to Ross Kirsling from comment #4) > Alright, so here is a test that hits the non-local read-only path, fails before your patch, and succeeds with it: > > (function () { > let count = 0; > > const x = 42; > function captureX() { return x; } > > try { > x -= (() => count++)(); > } catch { > if (count === 1) > return; > } > throw new Error('failed!'); > })(); > > > I...actually don't understand why. I just adapted it from here: https://github.com/WebKit/webkit/blob/master/JSTests/stress/const-semantics.js#L125-L129 (o.0) funky stuff :P Thanks for the help! :) Created attachment 396098 [details]
Patch
Comment on attachment 396098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396098&action=review r=me > JSTests/stress/const-read-modify-assignment-eval-rhs-before-exception.js:27 > +Object.defineProperty(globalThis, "testConstReadModifyAssignmentRHSEvalBeforeException", { Hmm, if you don't remove this test you should probably at least change the property name, since it's a `!var.local() && !var.isReadOnly()` red herring. Comment on attachment 396098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396098&action=review >> JSTests/stress/const-read-modify-assignment-eval-rhs-before-exception.js:27 >> +Object.defineProperty(globalThis, "testConstReadModifyAssignmentRHSEvalBeforeException", { > > Hmm, if you don't remove this test you should probably at least change the property name, since it's a `!var.local() && !var.isReadOnly()` red herring. I'll just remove it :) Created attachment 396121 [details]
Patch
Committed r259905: <https://trac.webkit.org/changeset/259905> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396121 [details]. (In reply to Ross Kirsling from comment #4) > Alright, so here is a test that hits the non-local read-only path, fails > before your patch, and succeeds with it: > > (function () { > let count = 0; > > const x = 42; > function captureX() { return x; } > > try { > x -= (() => count++)(); > } catch { > if (count === 1) > return; > } > throw new Error('failed!'); > })(); > > > I...actually don't understand why. I just adapted it from here: > https://github.com/WebKit/webkit/blob/master/JSTests/stress/const-semantics. > js#L125-L129 This makes sense. !!.local() means it wasn’t captured. So it lives in a bytecode local for this function that’s being compiled. If .local() is false, it’s captured (In reply to Saam Barati from comment #12) > This makes sense. !!.local() means it wasn’t captured. So it lives in a > bytecode local for this function that’s being compiled. If .local() is > false, it’s captured Ahh, that's clear then, thanks! My brain was stuck on thinking about "local vs. global"... |
``` (function() { let count = 0; const x = 42; try { x += (() => ++count)(); } catch { } if (count !== 1) throw new Error(`expected 1 but got ${count}`); })(); ``` See NOTE 2 on <https://tc39.es/proposal-logical-assignment/#sec-assignment-operators-runtime-semantics-evaluation>.