Closed Bug 1160757 Opened 5 years ago Closed 1 year ago

Fix XrayWrapper's getOwnPropertyDescriptor trap

Categories

(Core :: XPConnect, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: evilpie, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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.
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.
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.
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.
(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.
Depends on: 1053271
Depends on: 1451017
If this is about XPCWrappedNative Xrays then this got fixed with bug 1053271?
This code https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/js/xpconnect/wrappers/XrayWrapper.cpp#1770-1804 seems to be missing from getOwnPropertyDescriptor.
Flags: needinfo?(peterv)
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.
Flags: needinfo?(bzbarsky)
CrossOriginXrayWrapper will go away in bug 1363208.  Then we can reevaluate the state here.
Depends on: 1363208
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.
Assignee: nobody → bzbarsky
Flags: needinfo?(peterv)
Flags: needinfo?(bzbarsky)
Priority: -- → P2
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/565a04cfb0e4
Make it clear that XrayWrapper::getPropertyDescriptor is unused.  r=bholley
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.