Closed Bug 980565 Opened 10 years ago Closed 10 years ago

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


(Core :: JavaScript: Standard Library, defect)

Not set





(Reporter: efaust, Assigned: efaust)



(Keywords: dev-doc-complete, Whiteboard: [js:p1])


(6 files, 1 obsolete file)

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.
Attached file HasOwnProperty.xpi
simplified addon from a failing jetpack test..
Assignee: nobody → efaustbmo
Attachment #8395026 - Flags: review?(jwalden+bmo)
Attachment #8395027 - Flags: review?(evilpies)
Attachment #8395027 - Flags: review?(evilpies) → review+
Keywords: dev-doc-needed
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]:


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+
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)
Obviously, the patch should also come with the removal of all the testDirectProxyHasOwn*.js jit-tests. They are now moot.
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+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.