es-shims / es5-shim

ECMAScript 5 compatibility shims for legacy (and modern) JavaScript engines
MIT License
7.12k stars 899 forks source link

[Fix] avoid sinking into endless loop cause by `getPrototypeOf` #458

Closed ambit-tsai closed 3 years ago

ambit-tsai commented 5 years ago

When we traverse the prototype chain of obj, as follows

var obj = {};
while (obj) {
  obj = Object.getPrototypeOf(obj);
}

The method getPrototypeOf always return a prototypeOfObject that cause the endless loop

ljharb commented 5 years ago

I'm not sure what you mean - the language itself prohibits cyclic prototype chains.

What code are you running that hits this error?

ambit-tsai commented 5 years ago

I'm not sure what you mean - the language itself prohibits cyclic prototype chains.

What code are you running that hits this error?

The Object.getPrototypeOf method returns an object's internal [[Prototype]], so that we can trace back to the end of prototype chain. Look at this code:

var obj = {};
while (obj) {
  obj = Object.getPrototypeOf(obj);
}

Under normal conditions, the loop will end when getPrototypeOf returns a null. When using es5-sham, I found that getPrototypeOf always return a prototypeOfObject.

ljharb commented 5 years ago

Can you provide a regression test that would have failed without your fix?

ljharb commented 5 years ago

Specifically, prototypeOfObject.__proto__ will be null, so the check would bail out prior to this point.

What browser are you seeing this in?

ambit-tsai commented 5 years ago

Specifically, prototypeOfObject.__proto__ will be null, so the check would bail out prior to this point.

What browser are you seeing this in?

Oh, I suddenly realized that proto === null is used to prevent this problem. But prototypeOfObject.__proto__ will be undefined, so that proto === null will be false. It's better to use proto === undefined or proto == null.

ambit-tsai commented 5 years ago

Specifically, prototypeOfObject.__proto__ will be null, so the check would bail out prior to this point.

What browser are you seeing this in?

IE8

ljharb commented 5 years ago

prototypeOfObject.__proto__ should be null - __proto__ should never be undefined - however, in IE 8, which lacks __proto__ support, I do see how it could be.

Could you add a test for this?

ambit-tsai commented 5 years ago

prototypeOfObject.__proto__ should be null - __proto__ should never be undefined - however, in IE 8, which lacks __proto__ support, I do see how it could be.

Could you add a test for this?

I take a screenshot for you. image Accessing a non existing property will get an undefined.

ljharb commented 5 years ago

Yes, i see that. I’m looking for an automated test that will prevent this from happening in the future.

ljharb commented 4 years ago

@ambit-tsai would you mind checking "allow edits" on the RHS of this PR?

ljharb commented 4 years ago

ping @ambit-tsai

ambit-tsai commented 4 years ago

@ambit-tsai would you mind checking "allow edits" on the RHS of this PR?

Sorry, I can't find "allow edits". Is it because I have deleted the fork repo?

ljharb commented 4 years ago

:-( yes, that would do it; deleting a fork when there's open PRs is an unrecoverable destructive action.