Closed Bug 1122552 Opened 5 years ago Closed 5 years ago

Introduce [[GetOwnProperty]] object op

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

No description provided.
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)
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)
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+
Blocks: 1122619
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+
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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"
Depends on: 1124947
You need to log in before you can comment on or make changes to this bug.