Do not add __proto__ to return value of Object.create(null).

[briandipalma]

Conversation behind this change: https://twitter.com/bterlson/status/558023598095360000
Before this change Object.create(null) and iterating the returned object, via Object.keys,
in IE8 would result in __proto__ incorrectly being returned as an own key.

[ljharb]

This was originally added in c608bb2 by @Benvie - I'd love his thoughts before merging this. Tests are passing, and I'm sure you've tested in IE 8, so it's probably fine, but I'd like to be cautious :-)

[michaelficarra]

LGTM.

[briandipalma]

This PR isn't required, sorry. The es5-shim library in the application I was testing yesterday was from before @Benvie's change.

All it had was https://github.com/es-shims/es5-shim/blob/master/es5-sham.js#L195 a line that created an object with an own property __proto__. Upgrading to the latest es5-shim fixes the issue.

The old line remains but I have no idea what JS engine goes down that path (Opera Mini?). Without an engine that goes down the first consequent I can't test that the change is necessary. I'm quite sure

createEmpty = function () {
  return { __proto__: null };
};

is wrong but it's an assumption. Maybe hasOwnProperty is false in that engine...

Sorry about this.

[michaelficarra]

I don't see that as a reason to close this. The comment says old IEs hit this path, and __proto__ is handled improperly.

• "__proto__" in Object.create(null) should be false.
• Object.create(null).__proto__ should be undefined, not null.

[ljharb]

@michaelficarra However, can Object.create(null).__proto__ = foo have an unexpected affect?

I'm hesitant to change this sham without extensive testing on multiple browsers.

[briandipalma]

IE8 executes the second clause. I guess @michaelficarra what you mean is that the second clause is incorrect and should not set __proto__ to null?

What about the first clause. Surely that's wrong too?

@ljharb You would have to go out of your way to execute your code snippet and the sham won't protect you from it. Also if it has an effect it will probably have the same effect in new browser engines too as they all support __proto__ now.

[michaelficarra]

@briandipalma Yes. And the first clause is only wrong when document doesn't exist. We can try to figure out how to handle that case better than we do now.

[briandipalma]

I think the document check is a precaution to prevent to code from going into the second clause as that uses the DOM to access another Realm's Object.

Maybe Rhinos and old V8s/node would go down that path but they should go down that path due to supportsProto?

I can't say I'm comfortable with changes here without understanding what old runtimes could be affect but at the same time I think those runtimes are not likely to still be in use.

I can delete the __proto__: null from the first clause if you guys want.

[ljharb]

fwiw, the es5-shim needs to work in as many old runtimes as possible, as far back as possible, regardless of usage.

[michaelficarra]

@briandipalma: I don't think you're understanding me. The first clause is fine, but only for the interpreters that get there because of supportsProto. The ones that get there because document doesn't exist should be handled differently somehow. Photoshop, for instance, is likely one of these interpreters.

edit: Also, this should be merged because it is a strictly positive change.

[briandipalma]

@michaelficarra Right, I understand now. Forgot about Photoshop, I figured the second condition was never hit by any environment. What is the different handling in that case? Create an object but delete any enumerable property?

[michaelficarra]

I'm not sure what to do in that case. The only thing I can think of would be horribly broken.

[briandipalma]

Will this be merged or should I close the PR?

[ljharb]

@briandipalma It won't be merged until all the open questions in this thread can be resolved, but I'm happy to merge it once I'm convinced that we've explored all the consequences of this change. I'm comfortable leaving it open, or closing it, as you prefer.

[ljharb]

Please rebase this branch freshly onto master. I'm still hoping for some clarity on why this line was necessary in the first place, but I think we can merge it anyways.

[dgreensp]

Ohh, I think I can guess why it was there in the first place. Ideally we want Object.getPrototypeOf(Object.create(null)) === null. However, here is the current sham for getPrototypeOf:

    Object.getPrototypeOf = function getPrototypeOf(object) {
        var proto = object.__proto__;
        if (proto || proto === null) {
            return proto;
        } else if (object.constructor) {
            return object.constructor.prototype;
        } else {
            return prototypeOfObject;
        }
    };

This code needs a tweak if Object.getPrototypeOf(Object.create(null)) is to return null instead of Object.prototype in IE<11.

[ljharb]

That makes a lot of sense, good catch @dgreensp

[ljharb]

@briandipalma would you mind freshly rebasing this?

@dgreensp if you want to update #321 instead along with the necessary getPrototypeOf tweak, please do!

[dgreensp]

Sure. There are no tests for this, right?

[ljharb]

I doubt it, but please do add some :-)

[dgreensp]

Where should they go? I think I only found shim (not sham) tests in the repo.

[ljharb]

They're intermixed in the other tests - see the conditional test here https://github.com/es-shims/es5-shim/blob/master/tests/spec/s-object.js#L4-L24 so that tests don't fail in an environment where it'd be impossible to polyfill it.

[briandipalma]

Travelling atm will fix this up at weekend

[ljharb]

Great, thanks!

[dgreensp]

I don't think it's possible in general to detect objects with a null proto, but we could detect:

• Objects with no __proto__ property at all in browsers that support __proto__.
• Objects we created with new Empty, which should be instanceof Empty.

[ljharb]

And certainly we also may not be able to do it cross-realm, but that's alright.

I'd say that you could detect it by doing 'hasOwnProperty' in {} === 'hasOwnProperty' in value though, right? Or even more robust, for (var key in {}) { break; } if (!key) { throw 'what do we do here'; } return key in {} === key in value ?

[dgreensp]

Ah, testing 'hasOwnProperty' in value might work.

I can't think of any JS environment where 'hasOwnProperty' in {} would be false, or where for (var key in {}) would find any keys. If there's such a browser, it would probably be good to be explicit about what happens then!

[ljharb]

Stock, you're correct. But ideally, the shims are robust enough that it takes absurdly extreme measures for someone to break them - ie, delete Object.prototype.hasOwnProperty shouldn't be enough to break the test, they'd have to delete every property on Object.prototype.

Now that I think about it, this might be the most robust altogether:

var key = '_____ some totally unguessable string ___' + Date.now();
var signal = {};
Object.prototype[key] = signal;
var hasObjectProto = value[key] === signal;
delete Object.prototype.key;
return !hasObjectProto;

[dgreensp]

In that vein, maybe we should check whether the value is instanceof Object before returning Object.prototype, else return null. (Cross-realm objects will return null.)

[ljharb]

It is a sham, after all, so best-effort is OK.

[hazzik]

So, the problem is that Object.keys returns __proto__.... Why not just blacklist it there?

[briandipalma]

So after the rebase these failures occurred, I'm thinking they are due to the ESLint 1.0.0 release but I'm not sure and I don't think this branch is the correct place to tackle such issues.

I'd say 1539 is the root cause of the errors.

I've created a PR to your shared ESLint config repo to try and fix this problem.

[dgreensp]

@ljharb gave me a "good to go" on PR #321, so I think it may supersede this PR at this point.

[ljharb]

@briandipalma Yes, I haven't finished testing #321 yet but I'm going to close this in favor of that one. I hope that's ok!

[briandipalma]

No worries, makes sense.