1156077 - Remove the getPropertyDescriptor trap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

getPropertyDescriptor is one of the last non-standard traps that we should probably remove. The concept of "getPropertyDescriptor" never materialized in ES6. Just removing the trap turned out to be somewhat tricky, because Xrays seem to require them. Instead I will try a piece by piece approach.

Aside: We have JS_GetPropertyDescriptor as well, which is not a good idea either. Especially because in some places we seem to assume that calling [[GetOwnProperty]] on the whole proto-chain of an object is the same as calling [[Get]]. This however is not true in some cases like TypedArrays, where all calls to the [[Get]] trap for indexed elements end.

Comment 1

[Tom S [:evilpie]]

So we are now at a point where removing getPropertyDescriptor is feasible. Right now my patches actually keep getPropertyDescriptor internally on Xray/Sandbox, because those need weird behavior. Maybe this can be resolved as well.

Comment 2

[Tom S [:evilpie]]

Attachment: Remove the getProperyDescriptor trap from JS

With bug 1053271 close to landing we can finally start working on removing this code. I think without XPCWN Xrays the only small wrinkle might be the DOM Sandbox code.

Comment 3

[Tom S [:evilpie]]

With bug 1363208 landing, we should finally be able to remove this. The only real user that I am aware of is the XPC Sandbox. Just removing SandboxProxyHandler::getPropertyDescriptor might work.

Comment 4

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0fab06ede039c86e6322faeabd2a18fdcff07c4

Comment 5

[Tom S [:evilpie]]

Attachment: Bug 1156077 - Remove the non-standard Proxy getPropertyDescriptor trap. r?bzbarsky,jorendorff

I am bit surprised myself, but just removing the getPropertyDescriptor trap seems to mostly work.
The only real special case here is the XPC Sandbox, which I changed to keep using its getPropertyDescriptorImpl.

testSetPropertyIgnoringNamedGetter.cpp didn't even really need its getPropertyDescriptor implementation.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46790408df7f
Remove the non-standard Proxy getPropertyDescriptor trap. r=bzbarsky,jorendorff

Comment 7

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/46790408df7f