1160757 - Fix XrayWrapper's getOwnPropertyDescriptor trap

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Tom S [:evilpie]]

I believe this trap doesn't do enough compared to the getPropertyDescriptor trap. E.g. when you have a same-origin Xray for Components, "interfaces" won't be resolved by getOwnPropertyDescriptor. 

This is actually already observable:
var x = sandbox.Components;
x.interfaces; // Removing this line changes hasOwnProperty from true to false
Object.prototype.hasOwnProperty(x, "interfaces")

I believe this is one of fundamental reasons why I am having problems with changing BaseProxyHandler::get away from getPropertyDescriptor to getOwnPropertyDescriptor.

Comment 1

[Bobby Holley (:bholley)]

I'm assuming this only applies to XPCWrappedNative Xrays, right? We're pretty close to getting rid of those, and their uses are quite slim at the moment, FWIW.

Comment 2

[Tom S [:evilpie]]

I think you are right, only XPCWrappedNative Xrays have a real implementation of resolveNativeProperty. Besides that there is the lookup on the holder that is done in getPropertyDescriptor, but not getOwnPropertyDescriptor.

Comment 3

[Tom S [:evilpie]]

Haven't we been close to removing XPCWNs for like a year or so? IIRC it involves moving everything from DOMClassInfo to the new bindings.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #3)
> Haven't we been close to removing XPCWNs for like a year or so? IIRC it
> involves moving everything from DOMClassInfo to the new bindings.

Yeah, nobody's really been working on it. The set of things we'd need to fix to get rid of XPCWN Xrays is smaller though, because most XPCWNs don't have a PreCreate hook, which means that we get XPCWN-per-global and we never have cross-compartment wrappers to them.

Comment 5

[Peter Van der Beken [:peterv]]

If this is about XPCWrappedNative Xrays then this got fixed with bug 1053271?

Comment 6

[Tom S [:evilpie]]

This code https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/js/xpconnect/wrappers/XrayWrapper.cpp#1770-1804 seems to be missing from getOwnPropertyDescriptor.

Comment 7

[Tom S [:evilpie]]

I still don't know the answer to previous question, i.e difference between getPropertyDescriptor and getOwnPropertyDescriptor. Another point is CrossOriginXrayWrapper. CrossOriginXrayWrapper claim everything is an own-property, but I think this is also true for target properties that aren't actually "own" properties. In that case we would have to fix this somehow by looking them up on the protochain, because the normal process of walking the protochain doesn't work for CrossOriginXrayWrapper, which censors that as well.

Comment 8

[Boris Zbarsky [:bzbarsky]]

CrossOriginXrayWrapper will go away in bug 1363208.  Then we can reevaluate the state here.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So after bug 1363208 the only difference between XrayWrapper::getPropertyDescriptor and XrayWrapper::getOwnPropertyDescriptor is that the latter checks the holder in addition to calling Traits::resolveOwnProperty. 

We have the following Traits::resolveOwnProperty impls:

1) JSXrayTraits::resolveOwnProperty -- already checks the holder.
2) DOMXrayTraits::resolveOwnProperty -- already checks the holder.
3) OpaqueXrayTraits::resolveOwnProperty -- does not check the holder, but always returns a valid descriptor or throws. 

That's it.  So in practice, afaict, XrayWrapper::getPropertyDescriptor has the same exact behavior as XrayWrapper::getOwnPropertyDescriptor.  Which is moderately bogus, of course, so I looked into how XrayWrapper::getPropertyDescriptor can get called nowadays:

* The only calls to getPropertyDescriptor that might land in XrayWrapper::getPropertyDescriptor are in Proxy::getPropertyDescriptor, XrayWrapper::get, and XrayWrapper::has.

* The only calls to has() and get() are from Proxy::has and Proxy::getInternal.

* Proxy::getPropertyDescriptor and Proxy::has check handler->hasPrototype() before doing the relevant calls and only call getPropertyDescriptor()/has() if that returns false.  And Xrays return true for hasPrototype().

So in particular, XrayWrapper::has is dead code.  Our existing code coverage data agrees.  See https://codecov.io/gh/mozilla/gecko-dev/src/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/js/xpconnect/wrappers/XrayWrapper.cpp#L2179

Proxy::getInternal only calls get() for the hasPrototype() case if hasOwn() claimed the property is own.  So we could just change XrayWrapper::get to use getOwnPropertyDescriptor.  And then XrayWrapper::getOwnPropertyDescriptor would be dead code.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1160757. Make it clear that XrayWrapper::getPropertyDescriptor is unused. r=bholley

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/565a04cfb0e4
Make it clear that XrayWrapper::getPropertyDescriptor is unused.  r=bholley

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/565a04cfb0e4