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] avoid sinking into endless loop cause by getPrototypeOf #458

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

ambit-tsai
Copy link

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

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
Copy link
Member

ljharb commented Oct 31, 2018

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
Copy link
Author

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
Copy link
Member

ljharb commented Oct 31, 2018

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

@ljharb
Copy link
Member

ljharb commented Oct 31, 2018

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

ljharb commented Oct 31, 2018

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
Copy link
Author

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
Copy link
Member

ljharb commented Oct 31, 2018

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

@ljharb
Copy link
Member

ljharb commented Jan 7, 2020

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

@ljharb
Copy link
Member

ljharb commented Mar 22, 2020

ping @ambit-tsai

@ambit-tsai
Copy link
Author

@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
Copy link
Member

ljharb commented Mar 23, 2020

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

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.

2 participants