Skip to content
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

[BUGFIX beta] Mandatory setter should check prototype descriptors. #12314

Merged
merged 1 commit into from
Oct 13, 2015

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Sep 9, 2015

Prior to this change, a setter that exists on the prototype of the object was not detected and was effectively clobbered.

This change updates handleMandatorySetter to check both the objects own descriptors and its prototypes descriptors.

Also removed the guard for Object.getOwnPropertyDescriptor since all supported platforms are ES5.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 9, 2015

@stefanpenner / @krisselden - For review...

@stefanpenner
Copy link
Member

r? @krisselden

@rwjblue
Copy link
Member Author

rwjblue commented Sep 18, 2015

@krisselden - Do you have a chance to review?

@stefanpenner
Copy link
Member

yes this looks good, we also have this issue during mixin application

@stefanpenner
Copy link
Member

quick chat with @krisselden, he raised that this does not walk the proto chain correctly. Which can cause us to accidentally miss + smash inherited accessors. This will likely cause another perf hit, but I suspect we may want to pursue it.

@krisselden im curious if you have any other ideas.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 21, 2015

Sounds good. Thanks for the feedback. I'll start with some more tests with deeper inheritance.

@krisselden
Copy link
Contributor

@rwjblue do you need help with this?

@rwjblue
Copy link
Member Author

rwjblue commented Sep 27, 2015

Updated with a few more tests (now we test 1, 2, and 3 level's of nesting) and an implementation that walks upwards until it either finds a descriptor for the given key name or reaches a null prototype.

Ready for review again (sorry for the delay)...

Prior to this change, a setter that exists on the prototype of the
object was not detected and was effectively clobbered.

This change updates `handleMandatorySetter` to check both the objects
own descriptors and its prototypes descriptors.

Also removed the guard for `Object.getOwnPropertyDescriptor` since all
supported platforms are ES5.
@rwjblue
Copy link
Member Author

rwjblue commented Sep 29, 2015

@krisselden / @stefanpenner - The previously mentioned concerns are addressed, this is ready for (hopefully) final review....

@stefanpenner
Copy link
Member

this looks good, I'm sad that this will likely make dev mode slower, but I am unclear how else to proceed.

rwjblue added a commit that referenced this pull request Oct 13, 2015
[BUGFIX beta] Mandatory setter should check prototype descriptors.
@rwjblue rwjblue merged commit ecbc94a into emberjs:master Oct 13, 2015
@rwjblue rwjblue deleted the check-prototype-for-setters branch October 13, 2015 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants