-
Notifications
You must be signed in to change notification settings - Fork 74
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
fix(ses): handle properties that are already override protected #1969
fix(ses): handle properties that are already override protected #1969
Conversation
7f2abe9
to
6357a8e
Compare
Can confirm this makes lockdown run successfully in Chrome Canary again! 🎉 |
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.
Preliminary review
As a matter of process, I would really prefer if independent changes were in separate commits. The change to the property override logic is great, but it seems it could easily stand in its own commit.
None of the existing tests broke, and I manually verified that things work as expected both on Chrome Canary, and on an earlier Chrome browser that has not implemented the iterators-helpers proposal.
You should be able to test this in the node-v8 nightly build.
6357a8e
to
a394a2a
Compare
b0f496f
to
d0de868
Compare
d0de868
to
d058f07
Compare
2cf8cf2
to
0449938
Compare
Ready for review again |
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.
I would be ok with this PR landing as-is but please consider adding messages to the test assertions.
Also: consider adding an entry to |
All test assertions are well commented. |
0449938
to
a95ca2b
Compare
Done. After merge though, please do look it over consistency with your style. Thanks. |
Thanks for addressing all the feedback! |
Fixes a bug initially reported by @FrederikBolding.
![image](https://private-user-images.githubusercontent.com/273868/296948309-b4296f35-dd84-416f-812c-7aa0b200f57a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYyMTMwOTAsIm5iZiI6MTczNjIxMjc5MCwicGF0aCI6Ii8yNzM4NjgvMjk2OTQ4MzA5LWI0Mjk2ZjM1LWRkODQtNDE2Zi04MTJjLTdhYTBiMjAwZjU3YS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMTA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDEwN1QwMTE5NTBaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1iMjU4NmQyOWU2ZGNlOWUxZTFiOTBiNTc0MDI0NWE4NmIwZmRhODU3N2ViMDU5MTMyNjIzNTQ3ZjllNzExOWQxJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.Weg4U3MjTGUtenGah5GOXcRK6r3mNoUo2oZQYkP1vEg)
Staged on #1974
closes: #1971
refs: #1655 #1968 https://github.com/tc39/proposal-iterator-helpers #1973
Description
The iterators-helpers proposal https://github.com/tc39/proposal-iterator-helpers is now shipping in Chrome Canary. As per spec, it makes two properties into accessors that act like data properties that protect against the override mistake, i.e., that enable the property to be overridden by assignment. However, our permits for these properties were written when we knew them to be data properties, are they currently are on the other engines we've tested on to date. Because of this discrepancy, SES failed to initialize on Chrome Canary.
The purpose of this PR is to provide a mechanism to tolerate such methods, so that SES initializes when it encounters an accessor method of this form.
During the
repairIntrinsics
phase of lockdown, before even vetted shims are supposed to run, for each property we said may be of this form, validate as best we can whether it indeed seems to be an accessor that protects from the override mistake, but otherwise emulates a data property. For those, temporarily turn it into the plain data property it seems to emulate. But writable, so it won't trigger the override mistake in that form. Note that this JS is not a conformant initial JS state, and is observable by vetted shims.Then, during the
enable-property-override
portion of thehardenIntrinsics
phase, after all vetted shims ran, those properties listed inenablements.js
will turn them (back) into accessors, but ones that we control. This PR also adds these two properties to all phases of enablements.It turned out that the
enablements
language could not use symbols as property names. This PR fixed that because it needed that feature.As a drive-by, the accessor functions installed by
enable-property-overrides.js
werefunction
functions. This PR changes them to concise methods, so they'll still have thethis
sensitivity they need, but omit theprototype
property and the[[Construct]]
behavior.Security Considerations
ses did exactly the right thing here. When the initial pre-ses state deviates from what the ses authors knew about as of that version of ses, we divide those changes into two categories:
So,
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
None of the existing tests broke, and I manually verified that things work as expected both on Chrome Canary, and on an earlier Chrome browser that has not implemented the iterators-helpers proposal.
I also exported the core logic as a function that I unit test.
Upgrade Considerations
This JS change was just encountered in Chrome Canary for now, but it is accepted into the language and will shortly ship on all platforms. Before then, all users of SES should upgrade to a SES incorporating this PR.