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

Investigate potential __proto__ issues. #226

Closed
jdalton opened this issue Mar 31, 2013 · 13 comments
Closed

Investigate potential __proto__ issues. #226

jdalton opened this issue Mar 31, 2013 · 13 comments
Labels

Comments

@jdalton
Copy link
Member

jdalton commented Mar 31, 2013

Investigate potential __proto__ issues. //cc @WebReflection

@jdalton
Copy link
Member Author

jdalton commented Mar 31, 2013

var a = {};a.__proto__ = [];Object.keys(a);
// => [] which is expected.

@WebReflection
Copy link

var a = Object.create(null);a.__proto__ = [];Object.keys(a);
// => ["__proto__"]

which is able to screw clone/copy operations being a bomb in the system able to break all instances

check this in Canary, Firefox, Nightly, or node.js

@jdalton
Copy link
Member Author

jdalton commented Mar 31, 2013

var a = Object.create(null);a.__proto__=[];Object.keys(a);
// => ['__proto__']; in Opera (the correct behavior).
// Safari/Chrome/Firefox are currently [], though Chrome has it fixed in Canary

@WebReflection
Copy link

precisely

@jdalton
Copy link
Member Author

jdalton commented Mar 31, 2013

which is able to screw clone/copy operations being a bomb in the system able to break all instances

This is the desired behavior, not a bomb. On an object with a null [[Prototype]], __proto__ is just a regular property. I'll look into making current Chrome and others consistent.

Also, how awesome is it that we both typed the exact same example code within seconds of each other. Wonder twin powers!

@jdalton
Copy link
Member Author

jdalton commented Mar 31, 2013

Ok, so since detecting if __proto__ has been manually added to an object with a null [[Prototype]] is impossible-ish on current non-ES6 compliant browsers, because __proto__ is still active on the object, I'm going to punt on addressing it. However, I believe addressing it would be trivial & have minimal perf impact if I could detect it.

To reiterate the behavior of var a=Object.create(null);a.__proto__=[];Object.keys(a); //['__proto__'] is desired.

@jdalton jdalton closed this as completed Mar 31, 2013
@jdalton
Copy link
Member Author

jdalton commented Mar 31, 2013

@sharifmacky ? ES6 draft has Object.prototype.__proto__ defined. When you create an object via Object.create(null) it has a null internal [[Prototype]] value and so does not inherit from Object.prototype. Any properties added to the object by assignment will be enumerable by default. When the __proto__ property is added to an object with a null [[Prototype]] it's just a regular property w/o any special abilities.

@WebReflection
Copy link

@jdalton the issue is not that Object.create(null) do not inherit __proto__ from Object.prototype and it threat it as a generic key, which is indeed expected and desired, the issue is that most mobile browsers have same issue Chrome has and the update there won't be available any soon.

Why This Is A Problem
Every method that should iterate over a generic object cannot work anymore as expected in a consistent way unless a check against key === '__proto__' is performed per each key before acting.

_.extend() is just an example, if the source object is a Dictionary (read Object.create(null)), and the __proto__ is a key, and the target object is a generic object that can have inheritance changed because of this key, which is undesired and never a problem until now thanks to non-enumerability of that key even in dictionaries, what should happen?

If Lo-Dash wants to be consistent with the known behavior:

  • each _.extend(target, source) call should verify what kind of objects those are
  • if target is Dictionary and source is Dictionary, copy everything without problems
  • if target is Object, and source is Dictionary, do not copy the possible __proto__ key preserving inheritance integrity and currently known behavior
  • if target is Object, and source is Object, and the Object.prototype descriptor of __proto__ is not the native one or it has been simply deleted, act as Dictionary & Dictionary, copy everything

Thanks TC39, We Gonna Have Bad Time
Are you willing to make a basic method such copy or extend aware of all possible scenarios introduced by the __proto__ concept which is configurable, then rewritable, potentially made enumerable if reconfigured, and not present in Dictionary only for the 50% of browsers out there?
As far as I know, IE11 is implementing that property in an even different way so, brace yourself, disaster and fear oriented development is coming due a property in the middle that is causing an exception to the standard that nobody really wants.

What To Do To Change This
AFAIK Allen confirmed that TC39 is aware of all these problems but it does not believe that libraries will drop its usage.

Of course libraries will not drop it until there is a standard able to replace same behavior.
In order to make TC39 change its mind we need to use a code that is relying into a non existent yet standard

var setPrototypeOf = Object.setPrototypeOf || function (o, p) {
  o.__proto__ = p;
  return o;
};

This method preserves easiness and semantic, allowing {__proto__:obj} behavior via Object.setPrototypeOf({}, obj) and once widely adopted could make it in favor of deprecating and removing from any spec the current __proto__ keyword ... which is a bomb in the system.

@jdalton
Copy link
Member Author

jdalton commented Mar 31, 2013

the issue is not that Object.create(null) do not inherit __proto__ from Object.prototype and it threat it as a generic key, which is indeed expected and desired, the issue is that most mobile browsers have same issue Chrome has and the update there won't be available any soon.

I'm not concerned w/ what the current non-spec'ed behavior is (we can't control that), I'm glad the spec'ed behavior is clearing up these issues.

Why This Is A Problem
Every method that should iterate over a generic object cannot work anymore as expected in a consistent way unless a check against key === 'proto' is performed per each key before acting.

Cannot work anymore? This has been an issue with __proto__ since the beginning and there hasn't been a pain point across libs that I'm aware of. The double underscore prefix/postfix does a good job of avoiding accidental issues.

Thanks TC39, We Gonna Have Bad Time

No, thanks to the TC39 __proto__ is getting better/more-consistent, we'll have a good time :D

If Lo-Dash wants to be consistent with the known behavior...

I've already explored this in my previous comment.
I may also simply prefix key names I use for data objects, in cachedContains for example.

As far as I know, IE11 is implementing that property in an even different way so, brace yourself, disaster and fear oriented development is coming due a property in the middle that is causing an exception to the standard that nobody really wants.

Let's not speculate on IE11 please.

This method preserves easiness and semantic, allowing {__proto__:obj} behavior via Object.setPrototypeOf({}, obj) and once widely adopted could make it in favor of deprecating and removing from any spec the current __proto__ keyword ... which is a bomb in the system

__proto__ is already used and a de facto standard. Standardizing is the right answer to clear up existing inconsistencies.

@WebReflection
Copy link

using Object.setPrototypeOf(), and everybody seems to agree on it, would solve this mess once for everyone without granting future problems for a property name, regardless which one is it, able to potentially destroy services.

The usage of a global Object method would be explicit and avoid everything we are discussing here .. so the point is, as I've written in my post, being stubborn for something that easy to solve nobody wants to.

So here my question: would you use Object.setPrototypeOf(obj, proto) if spec'd and if __proto__ will disappear?
I believe simply yes because there is no reason to not do it, IMHO, while there are tons of problems going forward with this messed up property.

@jdalton
Copy link
Member Author

jdalton commented Mar 31, 2013

using Object.setPrototypeOf(), and everybody seems to agree on it, would solve this mess once for everyone without granting future problems for a property name, regardless which one is it, able to potentially destroy services.

The sky is not falling. If it was a real pain point setPrototypeOf may have won out. As it is __proto__ getting standardized is a win, dealing w/ its current inconsistencies is a minor inconvenience.

@jdalton
Copy link
Member Author

jdalton commented Apr 1, 2013

_.extend() is just an example, if the source object is a Dictionary (read Object.create(null)), and the __proto__ is a key, and the target object is a generic object that can have inheritance changed because of this key, which is undesired and never a problem until now thanks to non-enumerability of that key even in dictionaries, what should happen?

I think that's a feature. If the source object has an enumerable __proto__ it may be some mixin or options object that is designed to set properties of the destination object.

So the only issue I can see is with our _.memoize and cachedContains data objects, which I'll address by prefixing keys. Methods like _.groupBy and _.countBy will be solved by convention.

jdalton added a commit that referenced this issue Aug 29, 2013
Former-commit-id: 55dee782acdd5e28229b1fcb7587424d3fdfd445
jdalton added a commit that referenced this issue Sep 1, 2013
Former-commit-id: 55dee782acdd5e28229b1fcb7587424d3fdfd445
jdalton added a commit that referenced this issue Sep 25, 2014
Former-commit-id: 55dee782acdd5e28229b1fcb7587424d3fdfd445
@lock
Copy link

lock bot commented Feb 13, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

2 participants