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.

Sign up for GitHub

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

Jump to bottom

Deoptimize ObjectExpression when a __proto__ property is present #4019

Merged
merged 5 commits into from Mar 29, 2021
Merged

Deoptimize ObjectExpression when a __proto__ property is present #4019

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c7ce37
Deoptimize ObjectExpression when a __proto__ property is present
marijnh Mar 26, 2021
0707e36
Integrate __proto__ detection with getPropertyMap
marijnh Mar 27, 2021
9e9d916
Merge branch 'master' into deoptimize-proto
lukastaegert Mar 29, 2021
87f59d3
Fix formatting
lukastaegert Mar 29, 2021
d3e7b1f
Merge branch 'master' into deoptimize-proto
lukastaegert Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/ast/nodes/ObjectExpression.ts
View file
Open in desktop
Expand Up @@ -54,6 +54,16 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE
super.bind();
// ensure the propertyMap is set for the tree-shaking passes
this.getPropertyMap();
if (
!this.hasUnknownDeoptimizedProperty &&
this.properties.some(prop => {
if (prop instanceof SpreadElement || prop.computed) return false;
if (prop.key instanceof Identifier) return prop.key.name == "__proto__";
return String((prop.key as Literal).value) == "__proto__";
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could actually be simplified by using the property map:

Suggested change
this.getPropertyMap();
if (
!this.hasUnknownDeoptimizedProperty &&
this.properties.some(prop => {
if (prop instanceof SpreadElement || prop.computed) return false;
if (prop.key instanceof Identifier) return prop.key.name == "__proto__";
return String((prop.key as Literal).value) == "__proto__";
})
const propertyMap = this.getPropertyMap();
if (
!this.hasUnknownDeoptimizedProperty &&
propertyMap.__proto__?.exactMatchRead

that means we check if there is at least one property in the object that can be explicitly determined to be __proto__ (and is not a setter). That would also automatically cover cases like this:

var foo = {['__proto__']: obj}

const getProto () => '__proto__' ;
var foo = {[getProto()]: obj}

One thing that is not covered, though is an unknown computed property that resolves to __proto__. If we would want to include that as well, then it gets kind of complicated.

Another alternative here would be to integrate this into getPropertyMap. Because in the end, __proto__ does not deoptimize everything, it just changes the prototype so that there may be additional "hidden" properties. So it would be equivalent to the very first property being an "unknown computed" property: All properties below are still valid.
Simplifying it further, one could just treat __proto__ like an unknown computed property when building the property map. And no explicit property deoptimization would need to take place. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Computed properties don't work for this feature (they create a regular property named __proto__, don't set the prototype), so that's not an issue.

This could definitely be smarter, this patch mostly tries to crudely patch the soundness hole. I could switch it over to using an unknown computed property, if you think that works. I'll look into it next week.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL: __proto__ needs to be a written string in object expressions. Ouch. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a patch that moves this to getPropertyMap as suggested.

) {
this.deoptimizeAllProperties();
}
}

// We could also track this per-property but this would quickly become much more complex
Expand Down
3 changes: 3 additions & 0 deletions test/form/samples/object-expression/proto-property/_config.js
View file
Open in desktop
@@ -0,0 +1,3 @@
module.exports = {
description: 'Deoptimize when __proto__ is used'
};
13 changes: 13 additions & 0 deletions test/form/samples/object-expression/proto-property/_expected.js
View file
Open in desktop
@@ -0,0 +1,13 @@
let proto = {
get a() { log(); }
};

let plainProto = {
__proto__: proto
};
if (plainProto.a) log("plainProto");

let quotedProto = {
"__proto__": proto
};
if (quotedProto.a) log("quotedProto");
18 changes: 18 additions & 0 deletions test/form/samples/object-expression/proto-property/main.js
View file
Open in desktop
@@ -0,0 +1,18 @@
let basic = {
a: false
};
if (basic.a) log("basic");

let proto = {
get a() { log(); }
};

let plainProto = {
__proto__: proto
};
if (plainProto.a) log("plainProto");

let quotedProto = {
"__proto__": proto
};
if (quotedProto.a) log("quotedProto");