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

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

({dev-doc-complete})

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p1])

Attachments

(6 attachments, 1 obsolete attachment)

Assignee

Description

5 years ago
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.
Posted file HasOwnProperty.xpi
simplified addon from a failing jetpack test..
Assignee

Comment 3

5 years ago
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8395026 - Flags: review?(jwalden+bmo)
Assignee

Comment 4

5 years ago
Attachment #8395027 - Flags: review?(evilpies)
Attachment #8395027 - Flags: review?(evilpies) → review+
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...
Attachment #8395028 - Flags: review?(bzbarsky) → review+
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?
Attachment #8395030 - Flags: review?(tomica+amo) → review+
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.
Attachment #8395026 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 10

5 years ago
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.
Attachment #8418996 - Flags: review?(jorendorff)
Assignee

Comment 11

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

Comment 12

5 years ago
Now with tests removed
Attachment #8418996 - Attachment is obsolete: true
Attachment #8418996 - Flags: review?(jorendorff)
Attachment #8421206 - Flags: review?(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.
Attachment #8421206 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/867fabab7a85
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.