Closed
Bug 1253866
Opened 8 years ago
Closed 8 years ago
Remove Proxy.create from crash tests
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
(Whiteboard: btpp-active)
Attachments
(2 files)
2.52 KB,
patch
|
bzbarsky
:
review-
|
Details | Diff | Splinter Review |
3.04 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #8727110 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → evilpies
Comment 1•8 years ago
|
||
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•8 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.
Comment 3•8 years ago
|
||
> 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•8 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");
Comment 5•8 years ago
|
||
> Changing this test like the following would probably make it approximate the old behavior.
Let's do that.
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8729047 -
Flags: review?(bzbarsky)
Comment 7•8 years ago
|
||
Comment on attachment 8729047 [details] [diff] [review] v2 - Remove Proxy.create from crash tests r=me
Attachment #8729047 -
Flags: review?(bzbarsky) → review+
Updated•8 years ago
|
Whiteboard: btpp-active
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/936dd659fb9a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•