610078 - JS_LookupPropertyById doesn't retrieve values out of proxies

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

This means that doing JS_LookupPropertyById(..., &v) won't return the value (it returns JSVAL_TRUE, which is pretty weird.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Right patch

Comment 3

[Andreas Gal :gal]

Comment on attachment 488624 [details] [diff] [review]
Right patch

As far as I understand this API, it is not supposed to get the value if it has to execute code for it. We don't run any getters here either (see XXX bad API note). Brendan?

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 488624 [details] [diff] [review]
Right patch

># HG changeset patch
># Parent cb65c801512aeda6a84d728efe87da49167f981f
># User Blake Kaplan <[e-mail]>
>Bug 610078 - Return the value when we found it on a proxy.

This is ok, it is not doing a get trap, it is doing getPropertyDescriptor -- a fundamental trap that returns a descriptor, which has a value property, which is the last-got or constant value. This is akin to how for native objects, this bad old API does a lookup (and resolve, which may define with an initial value) but not a getProperty (value-get).

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -3129,16 +3129,24 @@ LookupResult(JSContext *cx, JSObject *ob
> 
>         /* Peek at the native property's slot value, without doing a Get. */
>         if (obj2->containsSlot(shape->slot))
>             *vp = obj2->nativeGetSlot(shape->slot);
>         else
>             vp->setBoolean(true);
>     } else if (obj2->isDenseArray()) {
>         return js_GetDenseArrayElementValue(cx, obj2, id, vp);
>+    } else if (obj2->isProxy()) {

Avoid else after return by splitting the else-if chain above, like so:

..    } else {
..        if (obj2->isDenseArray())
..            return js_GetDenseArrayElementValue(cx, obj2, id, vp);
..        if (obj2->isProxy()) {

>+        AutoPropertyDescriptorRooter desc(cx);
>+        if (!JSProxy::getPropertyDescriptor(cx, obj2, id, false, &desc))
>+            return false;
>+        if (desc.attrs & JSPROP_SHARED)
>+            vp->setBoolean(true);
>+        else
>+            *vp = desc.value;
>     } else {
>         /* XXX bad API: no way to return "defined but value unknown" */
>         vp->setBoolean(true);

Common the vp->setBoolean(true) by earl-returning in the non-SHARED proxy case.

r=me with these picked.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

This needs to land for COWs to work properly again.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/583c43306d74