1253866 - Remove Proxy.create from crash tests

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tom S]

Attachment: Remove Proxy.create from crash tests

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8727110 [details] [diff] [review]
Remove Proxy.create from crash tests

>-var p = Proxy.create({getPropertyDescriptor: function() {return nodeList}});
>+var p = new Proxy({}, {get: function() {return nodeList}});

No, this is not the same at all.  The code fixed in the bug this is a regression test for was ending up crashing in has(), which this proxy totally won't do.

>-  a.__proto__ = Proxy.create({has: function() { throw new Error; }});
>+  a.__proto__ = new Proxy({}, {has: function() { throw new Error; }});

Is this equivalent?  That is, will it throw in the same place/way as the old thing, as opposed to somewhere else because some other hook is not implemented?

Comment 2

[Tom S]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Comment on attachment 8727110 [details] [diff] [review]
> Remove Proxy.create from crash tests
> 
> >-var p = Proxy.create({getPropertyDescriptor: function() {return nodeList}});
> >+var p = new Proxy({}, {get: function() {return nodeList}});
> 
> No, this is not the same at all.  The code fixed in the bug this is a
> regression test for was ending up crashing in has(), which this proxy
> totally won't do.
> 
You are right, but we can't really fix this test. Looking at the old stacktrace this was crashing while trying to lookup some property on "nodeList" treating it as a PropertyDescriptor. Probably can just remove this test.
> >-  a.__proto__ = Proxy.create({has: function() { throw new Error; }});
> >+  a.__proto__ = new Proxy({}, {has: function() { throw new Error; }});
> 
> Is this equivalent?  That is, will it throw in the same place/way as the old
> thing, as opposed to somewhere else because some other hook is not
> implemented?
You are right again, this is also not equivalent, but likely bunk as well. This lookup was happening when searching for __iterator__ for the for..in. This never happens nowadays, we just go to Proxy::enumerate. Which in this case means [[OwnKeys]] etc. I would argue for removal as well.

Please pick whatever you want. I just want to get rid of these.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Looking at the old stacktrace this was crashing while trying to lookup some property on "nodeList" treating it
> as a PropertyDescriptor.

OK... Would it make sense to return a nodeList here from getOwnProperty?  Or would that be testing something totally different?  And would that different thing be totally uninteresting?

>I would argue for removal as well.

For the __iterator__ thing, makes sense to me.

Comment 4

[Tom S]

(In reply to Boris Zbarsky [:bz] from comment #3)
> > Looking at the old stacktrace this was crashing while trying to lookup some property on "nodeList" treating it
> > as a PropertyDescriptor.
> 
> OK... Would it make sense to return a nodeList here from getOwnProperty?  Or
> would that be testing something totally different?  And would that different
> thing be totally uninteresting?
> 
Well p.x only invokes the "get" trap, but not "getOwnPropertyDescriptor". (The new proxies don't derive functionality from a set of fundamental traps) Changing this test like the following would probably make it approximate the old behavior.

var p = new Proxy({}, {getOwnPropertyDescriptor: function() {return nodeList}});
Reflect.getOwnPropertyDescriptor(p, "x");

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Changing this test like the following would probably make it approximate the old behavior.

Let's do that.

Comment 6

[Tom S]

Attachment: v2 - Remove Proxy.create from crash tests

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8729047 [details] [diff] [review]
v2 - Remove Proxy.create from crash tests

r=me

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/936dd659fb9a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/936dd659fb9a