1122552 - Introduce [[GetOwnProperty]] object op

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: introduce-getownpropertydescriptor

With this op at hand we can at least easily replace the getGenericAttributes hook. The bigger goal however is for this to help getting rid of the lookup* ops.

Comment 2

[Tom S [:evilpie]]

Attachment: introduce-getownpropertydescriptor-browser

Trivial, because none of these implement the getGenericAttributes or lookup* ops, so we don't change any behavior by falling back to the native behavior.

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8550306 [details] [diff] [review]
introduce-getownpropertydescriptor

Niko, can you look if the hook for typedobjects implements the right semantics?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8550307 [details] [diff] [review]
introduce-getownpropertydescriptor-browser

Mmm, looking forward to proxy ops and this stuff aligning more...

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8550306 [details] [diff] [review]
introduce-getownpropertydescriptor

Review of attachment 8550306 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you. This is good stuff.

::: js/src/builtin/TypedObject.cpp
@@ +1994,5 @@
> +TypedObject::obj_getOwnPropertyDescriptor(JSContext *cx, HandleObject obj, HandleId id,
> +                                          MutableHandle<JSPropertyDescriptor> desc)
> +{
> +    MOZ_ASSERT(obj->is<TypedObject>());
> +    Rooted<TypedObject *> typedObj(cx, &obj->as<TypedObject>());

Delete the assertion: JSObject::as<T> contains an equivalent assertion.

@@ +2012,5 @@
> +      {
> +        uint32_t index;
> +        if (js_IdIsIndex(id, &index)) {
> +            if (!obj_getArrayElement(cx, typedObj, descr, index, desc.value()))
> +                return false;

I don't see a bounds check here (or in the original obj_getGenericAttributes code). There should be one, right?

Comment 6

[Tom S [:evilpie]]

Comment on attachment 8550306 [details] [diff] [review]
introduce-getownpropertydescriptor

Review of attachment 8550306 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TypedObject.cpp
@@ +2012,5 @@
> +      {
> +        uint32_t index;
> +        if (js_IdIsIndex(id, &index)) {
> +            if (!obj_getArrayElement(cx, typedObj, descr, index, desc.value()))
> +                return false;

I hoped Niko could comment on this, but I guess the idea is that TypedObjects claim ownership of all indexes. I actually asked for a spec that clarifies this, but there seems to be none.

Comment 7

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8550306 [details] [diff] [review]
introduce-getownpropertydescriptor

Review of attachment 8550306 [details] [diff] [review]:
-----------------------------------------------------------------

Looks right to me

::: js/src/builtin/TypedObject.cpp
@@ +2012,5 @@
> +      {
> +        uint32_t index;
> +        if (js_IdIsIndex(id, &index)) {
> +            if (!obj_getArrayElement(cx, typedObj, descr, index, desc.value()))
> +                return false;

I think it's best to consider these kinds of details a bit up in the air at the moment, but the intention was that typed objects (like typed arrays, iirc) claimed ownership of integer properties, yes.

Comment 8

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3aaa3b3a42

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce3aaa3b3a42

Comment 10

[Tom S [:evilpie]]

Comment on attachment 8550306 [details] [diff] [review]
introduce-getownpropertydescriptor

Review of attachment 8550306 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ScopeObject.cpp
@@ +566,5 @@
> +with_GetOwnPropertyDescriptor(JSContext *cx, HandleObject obj, HandleId id,
> +                              MutableHandle<JSPropertyDescriptor> desc)
> +{
> +    RootedObject actual(cx, &obj->as<DynamicWithObject>().object());
> +    return GetOwnPropertyDescriptor(cx, obj, id, desc);

Ups, this should be "actual" instead of "obj"