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 release] Only freeze params, hash when WeakMap available #14649

Merged
merged 1 commit into from Dec 17, 2016

Conversation

Projects
None yet
5 participants
@mixonic
Member

mixonic commented Nov 29, 2016

As of #14244 arguments to the helper methods are frozen in development mode. When those frozen values are then passed back to the rest of the rendering system they may have meta data tracked for them. In most modern browsers Ember uses a WeakMap to achieve this mapping, and the frozen object is not polluted with a property.

However in browsers which support Object.freeze but do not support WeakMap, we attempt to set a property on these frozen values resulting in the following error:

Cannot define property 'ember_meta': object is not extensible

Again, this is scoped to dev-mode and IE9, IE10, PhantomJS. However since PhantomJS is an extremely popular test runner and most apps run tests in dev mode, you may see this issue on continuous integration builds. It can be triggered by simple examples added as tests in this PR.

See also this issue: #14264

The real fix

The long-term fix is that Ember's object system should not use properties to track meta information. Ember already tracks meta via a WeakMap in browsers that support it, however we may be able to avoid adding a property lazily in many scenarios even without WeakMap. Completing the migration of Ember's object model to Glimmer2's reference/tag system would aid this effort, as would dropping support for browsers that do not support WeakMap.

This fix will take some significant time, and is part of a larger object model refactoring effort in Ember.

The quick fix

The quick fix centers on the fact that this frozen objects are really only a dev-mode dummy-check. We only need to freeze objects in browsers where we can reliably use those frozen objects through the whole rendering system. Currently Ember freezes argument object in dev mode as long as Object.freeze is supported- We should change it to only freeze if both Object.freeze and WeakMap are both present. IE9, IE10, PhantomJS would simply never freeze, however modern environments like Safari or Chrome would.

This fix should be achievable in a point release for 2.10. It is low-hanging fruit that significantly improves the backward compatibility of the 2.10 releases.

The workaround

Lastly, some apps may be eager to update to 2.10 and don't want to wait on a fix. As a work-around for this issue you can allocate a new object from a passed-in frozen object. For example:

export default Ember.Helper.helper(params => params.slice())

This of course may have a performance impact in your application, so use this workaround wisely.

@dwickern

This comment has been minimized.

Show comment
Hide comment
@dwickern

dwickern Nov 30, 2016

A different quick fix idea: You could make sure ember_meta exists before freezing if WeakMap is not supported. Then you would still have the advantages of freezing in those browsers.

dwickern commented Nov 30, 2016

A different quick fix idea: You could make sure ember_meta exists before freezing if WeakMap is not supported. Then you would still have the advantages of freezing in those browsers.

@mixonic mixonic changed the title from Reproduction for frozen params, hash bug to [BUGFIX release] Only freeze params, hash when WeakMap available Dec 11, 2016

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Dec 11, 2016

Member

@dwickern We don't want to eagerly apply meta- it isn't always going to be needed and has a cost. The fix for only freezing when WeakMap is present seems sufficient.

I'e updated this PR with an implementation of the WeakMap check. Need to pair up with @chancancode and/or @rwjblue on it. This fix may need to be applied in other additional spots.

Member

mixonic commented Dec 11, 2016

@dwickern We don't want to eagerly apply meta- it isn't always going to be needed and has a cost. The fix for only freezing when WeakMap is present seems sufficient.

I'e updated this PR with an implementation of the WeakMap check. Need to pair up with @chancancode and/or @rwjblue on it. This fix may need to be applied in other additional spots.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Dec 17, 2016

Member

@rwjblue this is good to go I believe, per our discussion last weekend. I'll get the change to default empty objects landed in Glimmer as well.

Member

mixonic commented Dec 17, 2016

@rwjblue this is good to go I believe, per our discussion last weekend. I'll get the change to default empty objects landed in Glimmer as well.

@rwjblue rwjblue merged commit d744508 into emberjs:master Dec 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwjblue rwjblue deleted the mixonic:frozen-helper-arguments branch Dec 17, 2016

@kellyselden

This comment has been minimized.

Show comment
Hide comment
@kellyselden

kellyselden Jan 5, 2017

Contributor

@rwjblue @mixonic I think this was missed in the last point release.

Contributor

kellyselden commented Jan 5, 2017

@rwjblue @mixonic I think this was missed in the last point release.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jan 5, 2017

Member

@kellyselden It is not in the release branch. Additionally glimmerjs/glimmer-vm#361 should find its way to release if possible.

Member

mixonic commented Jan 5, 2017

@kellyselden It is not in the release branch. Additionally glimmerjs/glimmer-vm#361 should find its way to release if possible.

@kellyselden

This comment has been minimized.

Show comment
Hide comment
@kellyselden

kellyselden Jan 5, 2017

Contributor

@mixonic OK my mistake.

Contributor

kellyselden commented Jan 5, 2017

@mixonic OK my mistake.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jan 5, 2017

Member

@kellyselden I think I am agreeing with you :-) We should get both this and the Glimmer change into the next point release.

Member

mixonic commented Jan 5, 2017

@kellyselden I think I am agreeing with you :-) We should get both this and the Glimmer change into the next point release.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jan 6, 2017

Member

@kellyselden 2.11 stable will release on Monday, we're thinking this can land in 2.11 instead of 2.10.x since we've not seen so many devs run into this. It means the fix will still get in before the LTS release, which is a big deal.

Member

mixonic commented Jan 6, 2017

@kellyselden 2.11 stable will release on Monday, we're thinking this can land in 2.11 instead of 2.10.x since we've not seen so many devs run into this. It means the fix will still get in before the LTS release, which is a big deal.

chancancode added a commit that referenced this pull request Jan 10, 2017

chancancode added a commit that referenced this pull request Jan 10, 2017

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jan 10, 2017

Member

@kellyselden you are correct! somehow I missed it when cutting the last release. I'll make sure this lands in 2.11

Member

chancancode commented Jan 10, 2017

@kellyselden you are correct! somehow I missed it when cutting the last release. I'll make sure this lands in 2.11

chancancode added a commit that referenced this pull request Jan 10, 2017

[BUGFIX beta] Bump glimmer-engine
Related to #14649

(cherry picked from commit 3d49c8a)

sclatter added a commit to sclatter/ember.js that referenced this pull request Jan 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment