Eric Faust [:efaust] Assignee
	Description • 10 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.

	Eric Faust [:efaust] Assignee
	Comment 1 • 10 years ago

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

	Tomislav Jovanovic :zombie
	Comment 2 • 10 years ago

simplified addon from a failing jetpack test..

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 10 years ago

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...

	Tomislav Jovanovic :zombie
	Comment 8 • 10 years ago

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?

	Jeff Walden [:Waldo]
	Comment 9 • 10 years ago

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.

	Eric Faust [:efaust] Assignee
	Comment 10 • 10 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.

	Eric Faust [:efaust] Assignee
	Comment 11 • 10 years ago

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

	Eric Faust [:efaust] Assignee
	Comment 12 • 10 years ago

Now with tests removed

	Jason Orendorff [:jorendorff]
	Comment 13 • 10 years ago

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.

	Eric Faust [:efaust] Assignee
	Comment 14 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 15 • 10 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 16 • 9 years ago

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