980565 - ES6 Proxies: There's no such thing as [[HasOwnProperty]]

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Eric Faust [:efaust]]

We have a hook implemented for this. It calls back into JS. Spec has no mention of such a hook. It should be removed.

We will try here to remove it alltogether, though we will have to see how plausible that is, in light of the hasPrototype stuff.

Comment 1

[Eric Faust [:efaust]]

https://tbpl.mozilla.org/?tree=Try&rev=af75b46cc91e

Comment 2

[Tomislav Jovanovic :zombie]

Attachment: HasOwnProperty.xpi

simplified addon from a failing jetpack test..

Comment 3

[Eric Faust [:efaust]]

Attachment: Remove the hasOwn hook as well as trivial uses around xpconnect

Comment 4

[Eric Faust [:efaust]]

Attachment: Remove the hasOwn hook from js/ipc

Comment 5

[Eric Faust [:efaust]]

Attachment: remove the hasOwn hook from DOM proxies and the global window proxy

Comment 6

[Eric Faust [:efaust]]

Attachment: Modify Jetpack to account for the changes in proxy API

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8395028 [details] [diff] [review]
remove the hasOwn hook from DOM proxies and the global window proxy

This is ok as far as it goes, but please remove the codegen for hasOwn as well?  We should be flagging all that codegenned stuff as MOZ_OVERRIDE to catch issues like that.  :(  Bonus points for a followup to fix that.

And I should note that hasOwn was faster than getOwnPropertyDescriptor, since it doesn't have to do native-to-JS conversions.  I realize that there is no such trap in the spec, but it's not clear to me that that our internal API must match the spec here.  Having a hasOwn derived trap that defaults to doing getOwnPropertyDescriptor and otherwise can do something faster actually makes some sense...

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8395030 [details] [diff] [review]
Modify Jetpack to account for the changes in proxy API

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

r+ 

Eric, my understanding was that you were gonna switch those to direct proxies. did you just opt do minimal changes for this bug (and leave that to us), or have you changed your mind about what should be done here?

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8395026 [details] [diff] [review]
Remove the hasOwn hook as well as trivial uses around xpconnect

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

::: js/src/builtin/Object.cpp
@@ +662,2 @@
>              return false;
> +        args.rval().setBoolean(!!desc.object());

Add an isUndefined() method to JSPropertyDescriptor and JS::PropertyDescriptorOperations, and use that instead of !!desc.object().

::: js/src/jsproxy.cpp
@@ +2359,2 @@
>          return false;
> +    if (*bp = !!desc.object())

Needs extra parentheses to avoid compiler warnings.

Comment 10

[Eric Faust [:efaust]]

Attachment: Just remove Proxy.[[HasOwnProperty]]

I did some testing on this. It's /way/ slower to not have the hasOwn trap for DOM stuff. Like measures in multiples, not percentages. As such, let's just clean up the stuff from the scripted proxy point of view.

Comment 11

[Eric Faust [:efaust]]

Obviously, the patch should also come with the removal of all the testDirectProxyHasOwn*.js jit-tests. They are now moot.

Comment 12

[Eric Faust [:efaust]]

Attachment: Just remove Proxy.[[HasOwnProperty]] v2

Now with tests removed

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8421206 [details] [diff] [review]
Just remove Proxy.[[HasOwnProperty]] v2

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

::: js/src/jit-test/tests/proxy/testDirectProxyHasOwn1.js
@@ -1,5 @@
> -// Forward to the target if the trap is not defined
> -var proxy = Proxy(Object.create(Object.create(null, {
> -    'foo': {
> -        configurable: true
> -    }

Just rename this test; don't remove it. It should still pass and we should have something testing Object#hasOwnProperty on proxies.

For that matter, a test that hasOwnProperty() calls exactly the `getOwnPropertyDescriptor` trap (and not the `has` trap) would be nice.

Comment 14

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/867fabab7a85

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/867fabab7a85

Comment 16

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Deprecated_and_obsolete_features#Proxy
https://developer.mozilla.org/en-US/Firefox/Releases/33#JavaScript