Expose Object::DefineOwnProperty on the API

Expose Object::DefineOwnProperty on the API

This is equivalent to calling Object.defineProperty on an Object.

BUG=chromium:475206
R=adamk@chromium.org,dcarney@chromium.org,verwaest@chromium.org
LOG=y

[dcarney, 2015-04-15 08:31:17]

lgtm

https://codereview.chromium.org/1073953004/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1073953004/diff/1/src/api.cc#newcode3449
src/api.cc:3449: auto obj = Utils::OpenHandle(this);
most of these got renamed to 'self'

https://codereview.chromium.org/1073953004/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/1073953004/diff/1/src/v8natives.js#newcode725
src/v8natives.js:725: true);
spacing seems funny

https://codereview.chromium.org/1073953004/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1073953004/diff/1/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:13425: v8::Local<v8::Value> val =
maybe_val.ToLocalChecked();
as a rule, always try to hide Maybes and MaybeLocals, just use ToLocalChecked
directly on the result

[jochen (gone - plz use gerrit), 2015-04-15 08:35:19]

https://codereview.chromium.org/1073953004/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1073953004/diff/1/src/api.cc#newcode3449
src/api.cc:3449: auto obj = Utils::OpenHandle(this);
On 2015/04/15 at 08:31:17, dcarney wrote:
> most of these got renamed to 'self'

done

https://codereview.chromium.org/1073953004/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/1073953004/diff/1/src/v8natives.js#newcode725
src/v8natives.js:725: true);
On 2015/04/15 at 08:31:17, dcarney wrote:
> spacing seems funny

it's what clang-format thinks it should be

https://codereview.chromium.org/1073953004/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1073953004/diff/1/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:13425: v8::Local<v8::Value> val =
maybe_val.ToLocalChecked();
On 2015/04/15 at 08:31:17, dcarney wrote:
> as a rule, always try to hide Maybes and MaybeLocals, just use ToLocalChecked
directly on the result

done

[adamk, 2015-04-15 16:31:54]

https://codereview.chromium.org/1073953004/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1073953004/diff/20001/include/v8.h#newcode2689
include/v8.h:2689: V8_WARN_UNUSED_RESULT MaybeLocal<Value> DefineObjectProperty(
Why not "DefineOwnProperty"? Also, it seems like it might be more useful to make
this non-throwing (in normal operation) and return a boolean, with true meaning
success and false meaning failure. This would better match up with
[[DefineOwnProperty]], which is what I was thinking of when I filed this bug.
The callers of this in the embedder are more like "spec code" than user code,
and the boolean is going to be easier to handle than sprinkling TryCatches and
ASSERTs around.

https://codereview.chromium.org/1073953004/diff/20001/include/v8.h#newcode2690
include/v8.h:2690: Local<Context> context, Local<Value> key, Local<Value> value,
Why not add a version that takes a uint32_t key? That'd match up with Set/Get,
and make this more convenient to use with Arrays (which is one of the places
where ForceSet currently doesn't work well).

https://codereview.chromium.org/1073953004/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1073953004/diff/20001/src/api.cc#newcode3452
src/api.cc:3452: i::Handle<i::Object> configurable =
Seems weird for this to not be auto as well...

https://codereview.chromium.org/1073953004/diff/20001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/1073953004/diff/20001/src/v8natives.js#newcod...
src/v8natives.js:719: DefineObjectProperty(obj, p, ToPropertyDescriptor({
I think you're entering too late, and should call DefineOwnProperty().  Please
add a test where the API method is called on an Array with the property
"length", as I think bad things will happen (see DefineArrayProperty for what
sort of special handling we do).

https://codereview.chromium.org/1073953004/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1073953004/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:13432: obj->DefineObjectProperty(env.local(),
v8_str("1"),
Please also add a test that passes in a v8::Integer::New(), as that's what some
of Blink's callers of ForceSet do now to handle indexed properties. 

You can ignore this comment if you add a uint32_t API.

[adamk, 2015-04-15 17:05:37]

I also have a higher-level concern about this approach: how much slower is this
than v8::Object::Set and ForceSet? From a recent read through Blink code, I
found few uses of Set that shouldn't be converted to DefineOwnProperty. But
shelling out to JS for each of those strikes me as possibly expensive.

[jochen (gone - plz use gerrit), 2015-04-15 18:59:43]

On 2015/04/15 at 17:05:37, adamk wrote:
> I also have a higher-level concern about this approach: how much slower is
this than v8::Object::Set and ForceSet? From a recent read through Blink code, I
found few uses of Set that shouldn't be converted to DefineOwnProperty. But
shelling out to JS for each of those strikes me as possibly expensive.

ForceSet() is also slow, as it us to deopt all code if we actually force a
property on an object. The other runtime methods we have that don't forcibly set
the property DCHECK() if you try to set a property that already exists. To have
a somewhat failsafe API, we need to go through javascript (or reimplement
Object.defineProperty in C++...)

The reason I picked DefineObjectProperty is because it's just that. I can also
invoke the full-blown Object.defineProperty() which would then also work on
arrays (the current one does not).

Also, using v8::Integer::New() as key doesn't work. The API expects a name as
key. Of course, we can convert everything to a string that is passed in but
that's again more expensive.

not throwing an exception and returning a bool instead sounds fine.

so... what exactly would you like the API to be? :)

[dcarney, 2015-04-15 19:02:56]

https://codereview.chromium.org/1073953004/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1073953004/diff/20001/include/v8.h#newcode2689
include/v8.h:2689: V8_WARN_UNUSED_RESULT MaybeLocal<Value> DefineObjectProperty(
On 2015/04/15 16:31:53, adamk wrote:
> Why not "DefineOwnProperty"? Also, it seems like it might be more useful to
make
> this non-throwing (in normal operation) and return a boolean, with true
meaning
> success and false meaning failure. This would better match up with
> [[DefineOwnProperty]], which is what I was thinking of when I filed this bug.
> The callers of this in the embedder are more like "spec code" than user code,
> and the boolean is going to be easier to handle than sprinkling TryCatches and
> ASSERTs around.

i think a  maybe<bool> is better than a meaningless maybelocal, but in either
case, the result is meaningless. only the presence of a result signals non
exceptional execution, whereas the absence signal something thrown.  what we
would really need here would be maybe<void>

[dcarney, 2015-04-15 19:04:51]

> not throwing an exception and returning a bool instead sounds fine.

that would be inconsistent with the entire rest of the api.  you have to allow
the exception to propagate

[adamk, 2015-04-15 19:11:08]

There are a bunch of different issues here, I guess I'll address each one
separately and then summarize at the end.

On 2015/04/15 18:59:43, jochen wrote:
> On 2015/04/15 at 17:05:37, adamk wrote:
> > I also have a higher-level concern about this approach: how much slower is
> this than v8::Object::Set and ForceSet? From a recent read through Blink code,
I
> found few uses of Set that shouldn't be converted to DefineOwnProperty. But
> shelling out to JS for each of those strikes me as possibly expensive.
> 
> ForceSet() is also slow, as it us to deopt all code if we actually force a
> property on an object.

But that's a pretty uncommon case for the majority use-case for this API, which
is to add own properties to newly-constructed objects. Whereas this patch
requires a call into JS even in the "fast" case.

> The other runtime methods we have that don't forcibly set
> the property DCHECK() if you try to set a property that already exists. To
have
> a somewhat failsafe API, we need to go through javascript (or reimplement
> Object.defineProperty in C++...)

It might be worth having a fast-path in C++ for the "no existing property,
extensible object" case. I don't think that's too much duplication, really.

> The reason I picked DefineObjectProperty is because it's just that. I can also
> invoke the full-blown Object.defineProperty() which would then also work on
> arrays (the current one does not).

v8::Array subclasses v8::Object, so adding methods to Object that have bad
behavior on Arrays doesn't seem like a good idea.

> Also, using v8::Integer::New() as key doesn't work. The API expects a name as
> key. Of course, we can convert everything to a string that is passed in but
> that's again more expensive.

The API as you have it here accepts a Local<Value>. If you added another call
that took a uint32_t I think it'd be quite appropriate to change this to
Local<Name>.

> not throwing an exception and returning a bool instead sounds fine.

Cool.

> so... what exactly would you like the API to be? :)

Sorry I wasn't more explicit in the original bug report, I sort of expected
there to be a bit more discussion on the bug before a CL appeared.

From a high level, I'd like it to be easily used as a drop-in replacement for
v8::Object::Set:

- It shouldn't be slower than Set()
- It should be easy to use on Arrays (the idiomatic embedder code for creating
arrays is a for loop from 0 to Length calling Set; this is almost always wrong)
- It shouldn't require any extra code in the caller over what Set() requires

From a low level, I was imagining:

Maybe<bool> DefineOwnProperty(Local<Context> context, Local<Name> key,
Local<Value> value, PropertyAttribute attribs = None);
Maybe<bool> DefineOwnProperty(Local<Context> context, uint32_t index,
Local<Value> value, PropertyAttribute attribs = None);

You could also imagine narrowing this and calling these "DefineDataProperty" or
something.

[adamk, 2015-04-15 19:13:15]

On 2015/04/15 19:04:51, dcarney wrote:
> > not throwing an exception and returning a bool instead sounds fine.
> 
> that would be inconsistent with the entire rest of the api.  you have to allow
> the exception to propagate

Why do you have to throw an exception in the first place? As I said above, I've
been thinking of this not as an equivalent to Object.defineProperty but as a
concrete implementation of [[DefineOwnProperty]].  [[DefineOwnProperty]] does
not throw, it returns a boolean indicating success or failure. I would expect
most callers of this to either ignore the return value or ASSERT(success).

[adamk, 2015-04-15 19:14:12]

On 2015/04/15 19:13:15, adamk wrote:
> On 2015/04/15 19:04:51, dcarney wrote:
> > > not throwing an exception and returning a bool instead sounds fine.
> > 
> > that would be inconsistent with the entire rest of the api.  you have to
allow
> > the exception to propagate
> 
> Why do you have to throw an exception in the first place? As I said above,
I've
> been thinking of this not as an equivalent to Object.defineProperty but as a
> concrete implementation of [[DefineOwnProperty]].  [[DefineOwnProperty]] does
> not throw, it returns a boolean indicating success or failure. I would expect
> most callers of this to either ignore the return value or ASSERT(success).

Oh, I think I see the confusion. Of course for cases where we have no choice but
to throw (say, worker termination) this can throw. What I'm suggesting is that
the underlying operation not throw when defining the property fails.

[dcarney, 2015-04-15 19:45:43]

On 2015/04/15 19:14:12, adamk wrote:
> Oh, I think I see the confusion. Of course for cases where we have no choice
but
> to throw (say, worker termination) this can throw. What I'm suggesting is that
> the underlying operation not throw when defining the property fails.

that's right. could be other things as well, as almost everything internally
seems to be capable of throwing, even when you might expect otherwise, but if we
had a Name version and a uint32_t version, and that hooked into
DefineOwnProperty internally with should_throw set to false, it'd probably be
less likely to throw.

[dcarney, 2015-04-15 19:52:45]

> It might be worth having a fast-path in C++ for the "no existing property,
> extensible object" case. I don't think that's too much duplication, really.

while this is totally reasonable, it's problematic as we really, really want to
limit the number of implementations of the same thing, which v8 has historically
done the absolute reverse of, and it's hell to deal with. the api should really
just go straight to js for things that are implemented in js.  It's just
unfortunate that DefineOwnProperty (or pretty much any builtin function) is done
in js.

[adamk, 2015-04-15 22:24:53]

On 2015/04/15 19:52:45, dcarney wrote:
> > It might be worth having a fast-path in C++ for the "no existing property,
> > extensible object" case. I don't think that's too much duplication, really.
> 
> while this is totally reasonable, it's problematic as we really, really want
to
> limit the number of implementations of the same thing, which v8 has
historically
> done the absolute reverse of, and it's hell to deal with. the api should
really
> just go straight to js for things that are implemented in js.  It's just
> unfortunate that DefineOwnProperty (or pretty much any builtin function) is
done
> in js.

I will certainly admit it's problematic. It's not clear to me it's any more
problematic than ForceSet's own brand of duplication. Though maybe we should
just write a ForceSet for elements.

[jochen (gone - plz use gerrit), 2015-04-20 07:30:34]

On 2015/04/15 at 22:24:53, adamk wrote:
> On 2015/04/15 19:52:45, dcarney wrote:
> > > It might be worth having a fast-path in C++ for the "no existing property,
> > > extensible object" case. I don't think that's too much duplication,
really.
> > 
> > while this is totally reasonable, it's problematic as we really, really want
to
> > limit the number of implementations of the same thing, which v8 has
historically
> > done the absolute reverse of, and it's hell to deal with. the api should
really
> > just go straight to js for things that are implemented in js.  It's just
> > unfortunate that DefineOwnProperty (or pretty much any builtin function) is
done
> > in js.
> 
> I will certainly admit it's problematic. It's not clear to me it's any more
problematic than ForceSet's own brand of duplication. Though maybe we should
just write a ForceSet for elements.

fine by me. would that work for you, Adam?

[adamk, 2015-04-20 17:30:54]

On 2015/04/20 07:30:34, jochen wrote:
> On 2015/04/15 at 22:24:53, adamk wrote:
> > On 2015/04/15 19:52:45, dcarney wrote:
> > > > It might be worth having a fast-path in C++ for the "no existing
property,
> > > > extensible object" case. I don't think that's too much duplication,
> really.
> > > 
> > > while this is totally reasonable, it's problematic as we really, really
want
> to
> > > limit the number of implementations of the same thing, which v8 has
> historically
> > > done the absolute reverse of, and it's hell to deal with. the api should
> really
> > > just go straight to js for things that are implemented in js.  It's just
> > > unfortunate that DefineOwnProperty (or pretty much any builtin function)
is
> done
> > > in js.
> > 
> > I will certainly admit it's problematic. It's not clear to me it's any more
> problematic than ForceSet's own brand of duplication. Though maybe we should
> just write a ForceSet for elements.
> 
> fine by me. would that work for you, Adam?

That definitely seems like a step up from where we are now. I can't think of any
case in Blink where we actually need the exact DefineOwnProperty semantics.

[jochen (gone - plz use gerrit), 2015-04-21 08:37:04]

On 2015/04/20 at 17:30:54, adamk wrote:
> On 2015/04/20 07:30:34, jochen wrote:
> > On 2015/04/15 at 22:24:53, adamk wrote:
> > > On 2015/04/15 19:52:45, dcarney wrote:
> > > > > It might be worth having a fast-path in C++ for the "no existing
property,
> > > > > extensible object" case. I don't think that's too much duplication,
> > really.
> > > > 
> > > > while this is totally reasonable, it's problematic as we really, really
want
> > to
> > > > limit the number of implementations of the same thing, which v8 has
> > historically
> > > > done the absolute reverse of, and it's hell to deal with. the api should
> > really
> > > > just go straight to js for things that are implemented in js.  It's just
> > > > unfortunate that DefineOwnProperty (or pretty much any builtin function)
is
> > done
> > > > in js.
> > > 
> > > I will certainly admit it's problematic. It's not clear to me it's any
more
> > problematic than ForceSet's own brand of duplication. Though maybe we should
> > just write a ForceSet for elements.
> > 
> > fine by me. would that work for you, Adam?
> 
> That definitely seems like a step up from where we are now. I can't think of
any case in Blink where we actually need the exact DefineOwnProperty semantics.

taking a step back, why can't we use Set()?

[jochen (gone - plz use gerrit), 2015-04-21 08:37:04]

On 2015/04/20 at 17:30:54, adamk wrote:
> On 2015/04/20 07:30:34, jochen wrote:
> > On 2015/04/15 at 22:24:53, adamk wrote:
> > > On 2015/04/15 19:52:45, dcarney wrote:
> > > > > It might be worth having a fast-path in C++ for the "no existing
property,
> > > > > extensible object" case. I don't think that's too much duplication,
> > really.
> > > > 
> > > > while this is totally reasonable, it's problematic as we really, really
want
> > to
> > > > limit the number of implementations of the same thing, which v8 has
> > historically
> > > > done the absolute reverse of, and it's hell to deal with. the api should
> > really
> > > > just go straight to js for things that are implemented in js.  It's just
> > > > unfortunate that DefineOwnProperty (or pretty much any builtin function)
is
> > done
> > > > in js.
> > > 
> > > I will certainly admit it's problematic. It's not clear to me it's any
more
> > problematic than ForceSet's own brand of duplication. Though maybe we should
> > just write a ForceSet for elements.
> > 
> > fine by me. would that work for you, Adam?
> 
> That definitely seems like a step up from where we are now. I can't think of
any case in Blink where we actually need the exact DefineOwnProperty semantics.

taking a step back, why can't we use Set()?

[jochen (gone - plz use gerrit), 2015-04-21 14:36:59]

to add some more context. ForceSet() is just wrong. If an object is frozen,
ForceSet will still set the value, but we don't deoptimize, so old optimized
code will just be wrong at this point.

In the end, the JS semantics is that Set() (and defineProperty) can fail. Since
we appear to be using ForceSet (and would like to use defineProperty) only in
situations where we know it can't fail, why not just use Set()?

[adamk, 2015-04-21 15:41:59]

On 2015/04/21 14:36:59, jochen wrote:
> to add some more context. ForceSet() is just wrong. If an object is frozen,
> ForceSet will still set the value, but we don't deoptimize, so old optimized
> code will just be wrong at this point.

This is why I argued for DefineOwnProperty to begin with: it has well-defined
semantics, unlike ForceSet.

> In the end, the JS semantics is that Set() (and defineProperty) can fail.
Since
> we appear to be using ForceSet (and would like to use defineProperty) only in
> situations where we know it can't fail, why not just use Set()?

In some circumstances, yes, [[DefineOwnProperty]] can fail. But the case we care
about for most uses in the API is like this (I'm writing in JS, but it's the
same in API):

Object.defineProperty(Object.prototype, 'foo', { get: function() {return 1;} });
Object.defineProperty(Object.prototype, 'bar', { get: function() {return 2;} });
var obj = {};
Object.defineProperty(obj, 'foo', {value: 1, configurable: true, writable: true,
enumerable: true});
obj.bar = 'hello';

The call to defineProperty will succeed, while the call to [[Set]] for bar will
fail (it'll find the accessor property on the prototype and throw as there's no
setter in this case).

For a newly-created object, it's not possible for [[DefineOwnProperty]] to fail
per spec (yes, it might fail in V8 due to memory allocation reasons, but that's
a different issue).

[jochen (gone - plz use gerrit), 2015-04-23 14:36:45]

so what about a defineProperty version that just CHECKs if we try something
that's not allowed to work?

Then we can skip all the checks implemented in JS right now...

[adamk, 2015-04-23 16:08:58]

On 2015/04/23 14:36:45, jochen wrote:
> so what about a defineProperty version that just CHECKs if we try something
> that's not allowed to work?
> 
> Then we can skip all the checks implemented in JS right now...

I think the clear downside to that is that we'd then have _three_ different ways
to set properties in the API, only one of them with fully defined language
semantics. Whereas just augmenting ForceSet for elements at least leaves us with
two. (maybe we should have this discussion on the bug?)

[adamk, 2015-04-23 18:33:10]

On 2015/04/23 16:08:58, adamk wrote:
> On 2015/04/23 14:36:45, jochen wrote:
> > so what about a defineProperty version that just CHECKs if we try something
> > that's not allowed to work?
> > 
> > Then we can skip all the checks implemented in JS right now...
> 
> I think the clear downside to that is that we'd then have _three_ different
ways
> to set properties in the API, only one of them with fully defined language
> semantics. Whereas just augmenting ForceSet for elements at least leaves us
with
> two. (maybe we should have this discussion on the bug?)

arv kindly points out that the spec _does_ actually have a higher-level abstract
operation matching this: "CreateDataProperty":

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createdataproperty

So we could wrap that with one more level and have a CreateDataProperty that
ASSERTs success (as you suggested).

[jochen (gone - plz use gerrit), 2015-05-26 16:48:15]

closing for https://codereview.chromium.org/1154233003