Remove Proxy.create from crash tests

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Created attachment 8727110 [details] [diff] [review]
Remove Proxy.create from crash tests
Attachment #8727110 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Assignee: nobody → evilpies
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?
Attachment #8727110 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 2

3 years ago
(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.
> 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.
(Assignee)

Comment 4

3 years ago
(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");
> Changing this test like the following would probably make it approximate the old behavior.

Let's do that.
(Assignee)

Comment 6

3 years ago
Created attachment 8729047 [details] [diff] [review]
v2 - Remove Proxy.create from crash tests
Attachment #8729047 - Flags: review?(bzbarsky)
Comment on attachment 8729047 [details] [diff] [review]
v2 - Remove Proxy.create from crash tests

r=me
Attachment #8729047 - Flags: review?(bzbarsky) → review+
Whiteboard: btpp-active

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/936dd659fb9a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.