ES6: Remove __proto__ setter poison pill

ES6: Remove __proto__ setter poison pill

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-set-object.prototype.__proto__

The __proto__ setter should be reusable on other objects.

BUG=v8:2804
LOG=y

[arv (Not doing code reviews), 2014-01-06 14:16:19]

ping

[arv (Not doing code reviews), 2014-01-16 14:51:22]

One more friendly ping.

[Michael Starzinger, 2014-01-16 16:04:53]

Andreas has a strong opinion about this poison pill. So I defer to his judgement
about the CL.

[Dmitry Lomov (no reviews), 2014-01-17 06:57:31]

On 2014/01/16 16:04:53, Michael Starzinger wrote:
> Andreas has a strong opinion about this poison pill. So I defer to his
judgement
> about the CL.

Here are my thoughts:
The blessed way to change a prototype of an object in ES6 is
Reflect.setPrototypeOf. __proto__ setter is buried there in Appendix B for
backward compatibility reasons.
However if we unpoison __proto__ setter without shipping Reflect.setPrototypeOf,
__proto__ setter is what the Internet will use.
Therefore we should only unpoison __proto__ setter after we have shipped
Reflect.setPrototypeOf.

[arv (Not doing code reviews), 2014-01-17 15:08:35]

On 2014/01/17 06:57:31, Dmitry Lomov (chromium) wrote:
> On 2014/01/16 16:04:53, Michael Starzinger wrote:
> > Andreas has a strong opinion about this poison pill. So I defer to his
> judgement
> > about the CL.
> 
> Here are my thoughts:
> The blessed way to change a prototype of an object in ES6 is
> Reflect.setPrototypeOf. __proto__ setter is buried there in Appendix B for
> backward compatibility reasons.
> However if we unpoison __proto__ setter without shipping
Reflect.setPrototypeOf,
> __proto__ setter is what the Internet will use.
> Therefore we should only unpoison __proto__ setter after we have shipped
> Reflect.setPrototypeOf.

Object.setPrototypeOf

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-object.setprototypeof

This one is trivial and we already have a pending patch for it.
https://code.google.com/p/v8/issues/detail?id=2675&q=setPrototypeOf&colspec=I...

I'm happy to take care of that but this one needs to come first.

[Dmitry Lomov (no reviews), 2014-01-17 15:20:25]

On 2014/01/17 15:08:35, arv wrote:
> On 2014/01/17 06:57:31, Dmitry Lomov (chromium) wrote:
> > On 2014/01/16 16:04:53, Michael Starzinger wrote:
> > > Andreas has a strong opinion about this poison pill. So I defer to his
> > judgement
> > > about the CL.
> > 
> > Here are my thoughts:
> > The blessed way to change a prototype of an object in ES6 is
> > Reflect.setPrototypeOf. __proto__ setter is buried there in Appendix B for
> > backward compatibility reasons.
> > However if we unpoison __proto__ setter without shipping
> Reflect.setPrototypeOf,
> > __proto__ setter is what the Internet will use.
> > Therefore we should only unpoison __proto__ setter after we have shipped
> > Reflect.setPrototypeOf.
> 
> Object.setPrototypeOf
> 
>
http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-object.setprototypeof
> 
> This one is trivial and we already have a pending patch for it.
>
https://code.google.com/p/v8/issues/detail?id=2675&q=setPrototypeOf&colspec=I...
> 
> I'm happy to take care of that but this one needs to come first.

Sorry I am not following. I suggest we land Object.setPrototypeOf first (the
patch is essentially:
function ObjectSetPrototypeOf(obj, proto) {
  %SetPrototype(obj, proto);
  return obj;
}
+ hooking it up in the right places - this patch does not need unpoisoning
__proto__ setter, does it?)

We can then unpoison __proto__ setter when we see fit.

[arv (Not doing code reviews), 2014-01-17 16:03:25]

On 2014/01/17 15:20:25, Dmitry Lomov (chromium) wrote:
> On 2014/01/17 15:08:35, arv wrote:
> > On 2014/01/17 06:57:31, Dmitry Lomov (chromium) wrote:
> > > On 2014/01/16 16:04:53, Michael Starzinger wrote:
> > > > Andreas has a strong opinion about this poison pill. So I defer to his
> > > judgement
> > > > about the CL.
> > > 
> > > Here are my thoughts:
> > > The blessed way to change a prototype of an object in ES6 is
> > > Reflect.setPrototypeOf. __proto__ setter is buried there in Appendix B for
> > > backward compatibility reasons.
> > > However if we unpoison __proto__ setter without shipping
> > Reflect.setPrototypeOf,
> > > __proto__ setter is what the Internet will use.
> > > Therefore we should only unpoison __proto__ setter after we have shipped
> > > Reflect.setPrototypeOf.
> > 
> > Object.setPrototypeOf
> > 
> >
>
http://people.mozilla.org/%257Ejorendorff/es6-draft.html#sec-object.setprotot...
> > 
> > This one is trivial and we already have a pending patch for it.
> >
>
https://code.google.com/p/v8/issues/detail?id=2675&q=setPrototypeOf&colspec=I...
> > 
> > I'm happy to take care of that but this one needs to come first.
> 
> Sorry I am not following. I suggest we land Object.setPrototypeOf first (the
> patch is essentially:
> function ObjectSetPrototypeOf(obj, proto) {
>   %SetPrototype(obj, proto);
>   return obj;
> }
> + hooking it up in the right places - this patch does not need unpoisoning
> __proto__ setter, does it?)
> 
> We can then unpoison __proto__ setter when we see fit.

You are right. I felt that removing the poison setter was a more natural
incremental step. 

I'll prepare a CL for setPrototypeOf.

[arv (Not doing code reviews), 2014-01-23 19:13:38]

On 2014/01/17 16:03:25, arv wrote:
> On 2014/01/17 15:20:25, Dmitry Lomov (chromium) wrote:
> > On 2014/01/17 15:08:35, arv wrote:
> > > On 2014/01/17 06:57:31, Dmitry Lomov (chromium) wrote:
> > > > On 2014/01/16 16:04:53, Michael Starzinger wrote:
> > > > > Andreas has a strong opinion about this poison pill. So I defer to his
> > > > judgement
> > > > > about the CL.
> > > > 
> > > > Here are my thoughts:
> > > > The blessed way to change a prototype of an object in ES6 is
> > > > Reflect.setPrototypeOf. __proto__ setter is buried there in Appendix B
for
> > > > backward compatibility reasons.
> > > > However if we unpoison __proto__ setter without shipping
> > > Reflect.setPrototypeOf,
> > > > __proto__ setter is what the Internet will use.
> > > > Therefore we should only unpoison __proto__ setter after we have shipped
> > > > Reflect.setPrototypeOf.
> > > 
> > > Object.setPrototypeOf
> > > 
> > >
> >
>
http://people.mozilla.org/%25257Ejorendorff/es6-draft.html#sec-object.setprot...
> > > 
> > > This one is trivial and we already have a pending patch for it.
> > >
> >
>
https://code.google.com/p/v8/issues/detail?id=2675&q=setPrototypeOf&colspec=I...
> > > 
> > > I'm happy to take care of that but this one needs to come first.
> > 
> > Sorry I am not following. I suggest we land Object.setPrototypeOf first (the
> > patch is essentially:
> > function ObjectSetPrototypeOf(obj, proto) {
> >   %SetPrototype(obj, proto);
> >   return obj;
> > }
> > + hooking it up in the right places - this patch does not need unpoisoning
> > __proto__ setter, does it?)
> > 
> > We can then unpoison __proto__ setter when we see fit.
> 
> You are right. I felt that removing the poison setter was a more natural
> incremental step. 
> 
> I'll prepare a CL for setPrototypeOf.

Now that setPrototypeOf landed I merged and retested this and it passes on all
the tests I know of (all + webkit) without any changes to the CL.

Removing the poison pill actually simplifies our code and it removes one branch
from all calls to getOwnPropertyDescriptor. It is also not compatible with other
browsers and does not match annex B, which if implemented should follow the spec
in annex B... and yada yada yada...

Can I get a review?

Thanks.

[arv (Not doing code reviews), 2014-02-05 21:21:24]

Andreas, any chance you can look at this?

[rossberg, 2014-02-06 11:53:21]

https://codereview.chromium.org/103343005/diff/1/test/mjsunit/proto-accessor.js
File test/mjsunit/proto-accessor.js (right):

https://codereview.chromium.org/103343005/diff/1/test/mjsunit/proto-accessor....
test/mjsunit/proto-accessor.js:42: // Check that any redefinition of the
__proto__ accessor works.
Maybe add a test case that redefines the setter to undefined and assignment to
__proto__ throws accordingly.

https://codereview.chromium.org/103343005/diff/1/test/mjsunit/proto-accessor....
test/mjsunit/proto-accessor.js:43: Object.defineProperty(Object.prototype,
"__proto__", { get:function() {
Nit: weird indentation

[arv (Not doing code reviews), 2014-02-06 15:43:47]

PTAL

I also added a test for deleted __proto__.

https://codereview.chromium.org/103343005/diff/1/test/mjsunit/proto-accessor.js
File test/mjsunit/proto-accessor.js (right):

https://codereview.chromium.org/103343005/diff/1/test/mjsunit/proto-accessor....
test/mjsunit/proto-accessor.js:42: // Check that any redefinition of the
__proto__ accessor works.
On 2014/02/06 11:53:22, rossberg wrote:
> Maybe add a test case that redefines the setter to undefined and assignment to
> __proto__ throws accordingly.

Done.

https://codereview.chromium.org/103343005/diff/1/test/mjsunit/proto-accessor....
test/mjsunit/proto-accessor.js:43: Object.defineProperty(Object.prototype,
"__proto__", { get:function() {
On 2014/02/06 11:53:22, rossberg wrote:
> Nit: weird indentation

Done.

[rossberg, 2014-02-06 16:08:46]

lgtm

[rossberg, 2014-02-06 16:16:31]

Landed (though git cl f'ed up commit notification).