1124935 - Remove LookupProperty from JS_GetPropertyDescriptor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

bug 1122619 comment 5
> But this function is a mess. In a separate patch maybe, could you try
> reimplementing it as follows, and see if it passes tests? I think it will...
> 
> static bool
> GetPropertyDescriptorById(JSContext *cx, HandleObject obj, HandleId id,
>                           MutableHandle<PropertyDescriptor> desc)
> {
>     RootedObject proto(cx);
>     for (RootedObject current(cx, obj); current; current = proto) {
>         if (current->is<ProxyObject>())
>             return Proxy::getPropertyDescriptor(cx, current, id, desc);
>         if (!GetOwnPropertyDescriptor(cx, current, id, desc))
>             return false;
>         if (desc.object())
>             return true;
>         if (!GetPrototype(cx, current, &proto))
>             return false;
>     }
>     MOZ_ASSERT(!desc.object());
>     return true;
> }
> 
> And while you're doing that, you could rename it to
> js::GetPropertyDescriptor(), move it to jsobj.h/cpp, and change the handful
> of other internal uses of JS_GetPropertyDescriptorById to use that.
>

Comment 2

[Tom S [:evilpie]]

Attachment: Reimplement GetPropertDescriptor

We should probably check if proxies still need a custom implementation of this trap.

There is a tiny difference in behavior here: LookupPropertyInline calls LookupOwnPropertyInline. Which checks if "done" is true and stops iterating. Thus before if you had x = new Int8Array(6) GetPropertyDescriptor(x, 50), we would always have an empty descriptor.

Comment 3

[Tom S [:evilpie]]

Thinking about it, GetDataProperty in AsmJSLink is probably wrong and should use something like LookupPropertyPure to avoid interacting with proxies along the proto chain?

Comment 4

[Eric Faust [:efaust]]

Comment on attachment 8559384 [details] [diff] [review]
Reimplement GetPropertDescriptor

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

LGTM

Comment 5

[Luke Wagner [:luke]]

We do want to call proxy hooks (specifically, we have to be able to get properties of the WindowProxy), just not scripted proxies (that are user-visible).  You make a good point, though, that JS_GetPropertyDescriptorById searches the proto chain and thus the IsScriptedProxy(obj) guard above is insufficient.  Will LookupPropertyPure call non-scripted proxy hooks?  Otherwise, we could use JS_GetOwnPropertyDescriptorById.

Comment 6

[Tom S [:evilpie]]

> Will LookupPropertyPure call non-scripted proxy hooks?
No, I just fails for non-native (or not unbodex) objects.
> Otherwise, we could use JS_GetOwnPropertyDescriptorById.
Probably good enough if you dond't care about properties from the prototype. Should probably be GetOwnPropertyDescriptor.

Comment 7

[Tom S [:evilpie]]

Oh this fails a test in DOM: dom/html/test/test_bug1081037.html. Basically it seems like document.registerElement expects the "has" trap to be invoked, which was a side-effect of the LookupProperty call.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think the test is just using that proxy trap to detect whether the implementation is doing the right things.

What the things are that the implementation should be doing is ... unclear.  But eventually it will need to be doing a DefinePropertyOrThrow on the custom prototype with the "constructor" property name.  Sadly, there is no way to do that via jsapi right now, and in particular, the define APIs we do have will happily redefine non-configurable props.  So the code is doing JS_GetPropertyDescriptor to see whether the prop is non-configurable and manually throw if it is.

What all this should do in practice, when the custom prototype is a scripted proxy, I have no idea.  I mean, long-term it should just get its defineProperty hook called or something...

For now, can you just switch the test to hasOwn if that's the trap you're now going to be calling?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Or I guess just switch it to getOwnPropertyDescriptor or whatever the proxy trap is for [[GetOwnProperty]].

Comment 10

[Tom S [:evilpie]]

Attachment: Fix document.registerElement patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8560041 [details] [diff] [review]
Fix document.registerElement patch

r=me

Fwiw, I think the has trap used to need to be there for the property get to happen at all...

Comment 12

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b220e74f92a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3054048c724b

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/3054048c724b