Introduce [[GetOwnProperty]] object op

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 8550306 [details] [diff] [review]
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.
Attachment #8550306 - Flags: review?(jorendorff)
(Assignee)

Comment 2

4 years ago
Created attachment 8550307 [details] [diff] [review]
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.
Attachment #8550307 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

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

Niko, can you look if the hook for typedobjects implements the right semantics?
Attachment #8550306 - Flags: feedback?(nmatsakis)
Comment on attachment 8550307 [details] [diff] [review]
introduce-getownpropertydescriptor-browser

Mmm, looking forward to proxy ops and this stuff aligning more...
Attachment #8550307 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Blocks: 1122619
(Assignee)

Updated

4 years ago
Blocks: 1124201
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?
Attachment #8550306 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 6

4 years ago
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 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.
Attachment #8550306 - Flags: feedback?(nmatsakis) → feedback+
https://hg.mozilla.org/mozilla-central/rev/ce3aaa3b3a42
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 10

4 years ago
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"
(Assignee)

Updated

4 years ago
Depends on: 1124947
You need to log in before you can comment on or make changes to this bug.