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

[FIX] Prevent setting of __nextSuper on frozen objects #5374

Closed

Conversation

rondale-sc
Copy link
Contributor

This fixes #5324

If the object is frozen we should not modify it. This includes the setting of __nextSuper which is used by _super.

@stefanpenner
Copy link
Member

this seems like a good work around, but since since a somewhat recent fix to super, we are actually apply this wrapper to methods and objects that we where not before.

I am slightly concerned that this will be even more of a performance drain then the current super is...

@mixonic any thoughts?

@rwjblue
Copy link
Member

rwjblue commented Aug 15, 2014

Object.isFrozen does not work in IE6 - IE8.

http://kangax.github.io/compat-table/es5/

@rondale-sc
Copy link
Contributor Author

@rwjblue So should we go with this solution we should polyfill Object.isFrozen and Object.freeze?

Been talking about this a bit and I'm not entirely certain why we are setting super then applying the function then resetting super to sup (which could more accurately be called originalSuper). This seems like we are putting a bandaid on a gunshot wound.

Also, I thought ie6-ie8 test suite would run on travis. Is that not correct?

Found a bit more context from #3683. Getting a better understanding of what is going on here. I have confirmed that contains worked in v1.4.0 and not on master.

This is interesting to me because this._super was still being modified in v.1.4.0 but did not trigger the error (which is the whole point of this PR ^_~). So trying to find out what changed between then and now.

So far I've been focusing on makeCtor and the o_defineProperty polyfill which both seem to have changed around this. Hopefully I'll have more information soon.

@mixonic
Copy link
Member

mixonic commented Aug 16, 2014

@stefanpenner can we stash __nextSuper on meta of the object instead of on the object? This would avoid the mutation.

@rondale-sc
Copy link
Contributor Author

@mixonic where would that happen? (tryin' to keep up. :)

@stefanpenner
Copy link
Member

@mixonic so currently we are super wrapping literally everything. Which means to store on the meta would mean to first create a meta for everything and this just continues to slow down our method calls.

I wonder if the older functionality was actually with less caveats then this one..

@mixonic
Copy link
Member

mixonic commented Aug 18, 2014

@stefanpenner only what has a possible _super is wrapped. The reason contains is problematic is that it is provided by Enumerable, then replaced by Array.

A few options:

  • Creating a meta for every array is definitely not good. However, creating a meta for anything that uses _super, then re-writing the Array to mixin some base of Enumerable that doesn't cause super wrappers to trigger seems reasonable. I can do this regardless, but after that maybe meta is a more reasonable solution.
  • I doubt we use defineProperty to avoid triggering an exception on frozen objects, and it is slow so not a great solution anyway.

For now I can fix the contains triggered issues on arrays by using a mixin that doesn't trigger any wrappers. I'd like to explore using meta more, but carefully.

@jamesarosen
Copy link

I don't know whether you've already considered this, but isSealed might be good to check as well. If the object doesn't already have the __nextSuper property, then even a sealed, unfrozen object will throw an exception on set.

mixonic added a commit to mixonic/ember.js that referenced this pull request Aug 19, 2014
Properties cannot be modified on a frozen object, which means even if
the functions to not alter the context, the super implementation itself
will raise an exception when a wrapped method calls (and __nextSuper is
set).

Part of the challenge here is that Ember wraps functions defensively- it
has no insight into if super will be called by a function and no tools
for hinting about non-super-calling functions. Ember also follows a
pattern of using hooks, so many methods are intended to be overridden
even if the override will never call super. Unfortunately this patterns
costs us.

But putting super on the meta we kill two birds with one stone. The
property does not need to use slow defineProperty (it is already not
enumerable on the object) and it can still be set regardless of an
object being frozen.

This is not expected to have an impact on performance. The number of
meta objects created during a run of the tests remains steady at about
22000 objects, and the number of requests for meta triples from 30k to
90k.

Fixes emberjs#4970, Fixes emberjs#5324, Fixes emberjs#5374
@stefanpenner
Copy link
Member

this may be fixed by #5444
@mixonic c/d?

@mixonic
Copy link
Member

mixonic commented Aug 21, 2014

Ignoring sets of __nextSuper on frozen objects is not a viable option. It would mean _super behaved differently on frozen and normal objects.

Closing in favor of #5464

@mixonic mixonic closed this Aug 21, 2014
@mixonic
Copy link
Member

mixonic commented Aug 21, 2014

@rondale-sc thank you for the research and effort! @stefanpenner's commit buys us some time to figure out the real cause.

@jamesarosen
Copy link

It looks like I was wrong about Object.seal. It prevents extension, but doesn't cause an exception, so won't cause a problem here.

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.

Cannot call contains on frozen Array
5 participants