Allow to define accessors on objects.

Allow to define accessors on objects.

Currently one can only define accessors on object templates.  This patch
allows to create accessors on the fly.

These accessors could control access to elements as well.  This element
support is somewhat rudimentary and may require future work (for example,
we probably don't want to convert index into a string.)

Committed: http://code.google.com/p/v8/source/detail?r=4714

[antonm, 2010-05-20 12:17:56]

Mads, Rico,

may you have a look?

[Mads Ager (chromium), 2010-05-21 07:49:57]

http://codereview.chromium.org/2123012/diff/1/6
File src/objects.cc (right):

http://codereview.chromium.org/2123012/diff/1/6#newcode2774
src/objects.cc:2774: || Top::MayNamedAccess(this, name, v8::ACCESS_SET));
Do the types match here? I think you check that HAS access is allowed and here
you assert that SET access is allowed. Should you be checking SET access below?

http://codereview.chromium.org/2123012/diff/1/6#newcode2879
src/objects.cc:2879: !Top::MayNamedAccess(this, name, v8::ACCESS_HAS)) {
Should you check for SET instead of HAS here?

http://codereview.chromium.org/2123012/diff/1/6#newcode2935
src/objects.cc:2935: if (result.IsProperty() && (result.IsReadOnly() ||
result.IsDontDelete())) {
Why do we check for DontDelete when setting an accessor property? I guess it is
up to your interpretation of the action of setting an accessor but to me that
does not seem like a deletion. If this is the behavior that we want, we should
make sure to document it.

Also, I don't understand the comment below. What are the two DefineAccessors you
are talking about? __defineGetter__ and __defineSetter__ defined accessors?
Example?

http://codereview.chromium.org/2123012/diff/1/6#newcode2936
src/objects.cc:2936: // That's somewhat unfortunate that two DefineAccessors
That's -> It is

http://codereview.chromium.org/2123012/diff/1/6#newcode2937
src/objects.cc:2937: // diff here, but another one performs DON_DELETE check
another one -> the other one

http://codereview.chromium.org/2123012/diff/1/6#newcode5966
src/objects.cc:5966: Handle<Object> number = Factory::NewNumberFromUint(index);
This code does not seem GC safe. |self| and |holder_handle| are raw pointers,
but the number allocation could cause a heap allocation and thereby a garbage
collection.

http://codereview.chromium.org/2123012/diff/1/6#newcode5994
src/objects.cc:5994: return 0;
NULL or maybe just undefined?

[antonm, 2010-05-21 10:20:52]

Thanks a lot for comments, Mads.

http://codereview.chromium.org/2123012/diff/1/6
File src/objects.cc (right):

http://codereview.chromium.org/2123012/diff/1/6#newcode2774
src/objects.cc:2774: || Top::MayNamedAccess(this, name, v8::ACCESS_SET));
On 2010/05/21 07:49:57, Mads Ager wrote:
> Do the types match here? I think you check that HAS access is allowed and here
> you assert that SET access is allowed. Should you be checking SET access
below?

You're right, thanks a lot for spotting this.  And see below as well.

http://codereview.chromium.org/2123012/diff/1/6#newcode2879
src/objects.cc:2879: !Top::MayNamedAccess(this, name, v8::ACCESS_HAS)) {
On 2010/05/21 07:49:57, Mads Ager wrote:
> Should you check for SET instead of HAS here?

I think you're right.  I copied this check from the code above (see ln.
2855-2856) in the patched code.

For now I changed checks to SET for both DefineAccessors.  Seems to work fine
with v8's (I cannot run layout tests on my linux box any more, going to
investigate), but there might be some implications to security which I miss.

http://codereview.chromium.org/2123012/diff/1/6#newcode2935
src/objects.cc:2935: if (result.IsProperty() && (result.IsReadOnly() ||
result.IsDontDelete())) {
On 2010/05/21 07:49:57, Mads Ager wrote:
> Why do we check for DontDelete when setting an accessor property? I guess it
is
> up to your interpretation of the action of setting an accessor but to me that
> does not seem like a deletion. If this is the behavior that we want, we should
> make sure to document it.
> 
> Also, I don't understand the comment below. What are the two DefineAccessors
you
> are talking about? __defineGetter__ and __defineSetter__ defined accessors?
> Example?

I started with the simple IsReadOnly.  However I had to add IsDontDelete for the
following scenario (covered in the first unit test):

Object.defineProperty(obj, 'foo', { ... configurable: false })

Now I think that overriding 'foo' should fail.  That seems consistent with ES5
(see 8.6.1, [[Configurable]] If false, attempts to delete the property, change
the property to be an accessor property, or change its attributes (other then
[[Value]]) will fail.

The principal question for me is if we'd like to treat API's accessors more like
JS accessor or JS funky properties.  My feeling is we currently sitting in
between, so I'd be glad to hear what you think the right way is.

And !configurable is encoded as DontDelete with our current PropertyAttribute
model.

I am talking about JSObject::DefineAccessor(String* name, bool is_getter,
JSFunction* fun, PropertyAttributes attributes) which used by both
Object.defineProperty and __defineGetter__.

Alas, Object.defineProperty and __defineGetter__ apparently do somewhat
different checks.

http://codereview.chromium.org/2123012/diff/1/6#newcode2936
src/objects.cc:2936: // That's somewhat unfortunate that two DefineAccessors
On 2010/05/21 07:49:57, Mads Ager wrote:
> That's -> It is

Done.

http://codereview.chromium.org/2123012/diff/1/6#newcode2937
src/objects.cc:2937: // diff here, but another one performs DON_DELETE check
On 2010/05/21 07:49:57, Mads Ager wrote:
> another one -> the other one

Done.

http://codereview.chromium.org/2123012/diff/1/6#newcode5966
src/objects.cc:5966: Handle<Object> number = Factory::NewNumberFromUint(index);
On 2010/05/21 07:49:57, Mads Ager wrote:
> This code does not seem GC safe. |self| and |holder_handle| are raw pointers,
> but the number allocation could cause a heap allocation and thereby a garbage
> collection.

Stupid me, thanks a lot.

http://codereview.chromium.org/2123012/diff/1/6#newcode5994
src/objects.cc:5994: return 0;
On 2010/05/21 07:49:57, Mads Ager wrote:
> NULL or maybe just undefined?

Thanks.  Here and in original GetPropertyWithCallback.

[Mads Ager (chromium), 2010-05-21 11:35:19]

http://codereview.chromium.org/2123012/diff/5001/6005
File src/objects.cc (right):

http://codereview.chromium.org/2123012/diff/5001/6005#newcode2935
src/objects.cc:2935: if (result.IsProperty() && (result.IsReadOnly() ||
result.IsDontDelete())) {
I think this requires a better comment. I still can't make sense of this only
based on the comment below. After talking to Rico about this, I agree that this
is the right thing to do because we are treating these API accessors as ES5
accessors. According to ES5, it is not allowed to overwrite any non-configurable
(DONT_DELETE) property with an accessor. I think you need to make this part of
the comment with a reference to the relevant part of ES5.

http://codereview.chromium.org/2123012/diff/5001/6005#newcode6042
src/objects.cc:6042: Handle<Object> holder_handle(holder);
This is not GC safe since the holder is wrapped in a handle *after* the call to
NewNumberFromUint.

[antonm, 2010-05-21 11:50:18]

http://codereview.chromium.org/2123012/diff/5001/6005
File src/objects.cc (right):

http://codereview.chromium.org/2123012/diff/5001/6005#newcode2935
src/objects.cc:2935: if (result.IsProperty() && (result.IsReadOnly() ||
result.IsDontDelete())) {
On 2010/05/21 11:35:19, Mads Ager wrote:
> I think this requires a better comment. I still can't make sense of this only
> based on the comment below. After talking to Rico about this, I agree that
this
> is the right thing to do because we are treating these API accessors as ES5
> accessors. According to ES5, it is not allowed to overwrite any
non-configurable
> (DONT_DELETE) property with an accessor. I think you need to make this part of
> the comment with a reference to the relevant part of ES5.

How do find new version?

I omitted comparison to another JSObject::DefineAccessor, but could restore it.

http://codereview.chromium.org/2123012/diff/5001/6005#newcode6042
src/objects.cc:6042: Handle<Object> holder_handle(holder);
On 2010/05/21 11:35:19, Mads Ager wrote:
> This is not GC safe since the holder is wrapped in a handle *after* the call
to
> NewNumberFromUint.

I am sorry I didn't recheck the rest of the code.

[Mads Ager (chromium), 2010-05-25 08:18:24]

LGTM

http://codereview.chromium.org/2123012/diff/11001/12005
File src/objects.cc (right):

http://codereview.chromium.org/2123012/diff/11001/12005#newcode2935
src/objects.cc:2935: // ECMA-262 forbids to turn a property into an accessor if
it's not
Replace ECMA-262 by ES5 to make the ES5/ES3 distinction clear here (both are
revisions of ECMA-262).

[antonm, 2010-05-25 12:06:38]

Thanks a lot for review, Mads.  Committing.

http://codereview.chromium.org/2123012/diff/11001/12005
File src/objects.cc (right):

http://codereview.chromium.org/2123012/diff/11001/12005#newcode2935
src/objects.cc:2935: // ECMA-262 forbids to turn a property into an accessor if
it's not
On 2010/05/25 08:18:24, Mads Ager wrote:
> Replace ECMA-262 by ES5 to make the ES5/ES3 distinction clear here (both are
> revisions of ECMA-262).

Of course.