1242072 - Change implementation of BaseProxyHandler::get() to follow ES6 [[Get]] specification

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Same idea as bug 1083211. The biggest advantage here is the removal of the getPropertyDescriptor call. Sadly we can expect that to cause issues with different proxies that want this call. We probably have to fix Xrays and Special Powers in some way.

Comment 1

[Tom S [:evilpie]]

Jason's idea of moving the old implementation of get to the derived classes that need it seems to work well. This try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2da189f15d7&selectedJob=15824783 only has a few failures in jetpack that I don't understand.

jetpack-package/addon-sdk/source/test/test-content-script.js.test
Cross Domain Iframe | COW fails : window.addEventListener is not a function - false == true

Comment 2

[Tom S [:evilpie]]

Looks like SandboxProxyHandler also needs the old get implementation. It's really a rather bad sign that all these proxies depend on calling getPropertyDescriptor. I assume somehow [[GetPrototype]] for these proxies isn't correct. Maybe the proto of those proxies aren't wrapped correctly?

Comment 3

[Tom S [:evilpie]]

Attachment: Change implementation of BaseProxyHandler::get() to follow ES6 [[Get]] specification

Comment 4

[Tom S [:evilpie]]

Attachment: Continue using getPropertyDescriptor for get in ScriptedIndirectProxyHandler

I am going to duplicate this code three times across different ProxyHandlers. The idea is that we can hopefully soon remove getPropertyDescriptor as a proxy trap. So to have these [[Get]] implementations continue to work we would move the getPropertyDescriptor code into the get methods.

Additionally I decided against just copy pasting the old BaseProxyHandler::get code, because it turns out half of it isn't even executed.

    if (!desc.getter()) {
        vp.set(desc.value());
        return true;
    }
    if (desc.hasGetterObject())
        return InvokeGetter(cx, receiver, ObjectValue(*desc.getterObject()), vp);

We basically have to cases: 1. We don't have a getter, so the first if is executed. 2. We have a getter and so the second if is executed.
This property should hold as long as all getPropertyDescriptor calls eventuelly end up js::GetOwnProperty, or only define relative sane descriptors by hand.
The rest of the function is totally dead. (This is also supported by https://www.tjhsst.edu/~jcranmer/c-ccov/mozilla/js/src/proxy/BaseProxyHandler.cpp.html)
The new code here follows the spec for [[Get]] pretty closely where possible.

Comment 5

[Tom S [:evilpie]]

Attachment: Continue using getPropertyDescriptor for get in XrayWrapper

Comment 6

[Tom S [:evilpie]]

Attachment: Continue using getPropertyDescriptor for get in Sandbox

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8711765 [details] [diff] [review]
Continue using getPropertyDescriptor for get in XrayWrapper

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

r=me with that assuming this is cut/paste.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +2136,5 @@
>      if (Traits::HasPrototype)
>        thisv = receiver;
>      else
>        thisv.setObject(*wrapper);
> +

Please add a comment here that this is cribbed from the old BaseProxyHandler::get implementation and can probably be simplified.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8711767 [details] [diff] [review]
Continue using getPropertyDescriptor for get in Sandbox

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

Same review comments as the other patch.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8711360 [details] [diff] [review]
Change implementation of BaseProxyHandler::get() to follow ES6 [[Get]] specification

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

Yes please.

Comment 10

[Tom S [:evilpie]]

(In reply to Bobby Holley (busy) from comment #7)
> Comment on attachment 8711765 [details] [diff] [review]
> Continue using getPropertyDescriptor for get in XrayWrapper
> 
> Review of attachment 8711765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that assuming this is cut/paste.
> 
> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> @@ +2136,5 @@
> >      if (Traits::HasPrototype)
> >        thisv = receiver;
> >      else
> >        thisv.setObject(*wrapper);
> > +
> 
> Please add a comment here that this is cribbed from the old
> BaseProxyHandler::get implementation and can probably be simplified.
This already is the simplified version.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6aef3196b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2969969174e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a34bffec910
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d7b2fb75cb

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f6aef3196b6
https://hg.mozilla.org/mozilla-central/rev/a2969969174e
https://hg.mozilla.org/mozilla-central/rev/4a34bffec910
https://hg.mozilla.org/mozilla-central/rev/60d7b2fb75cb