932320 - Use JS_DefineProperty for replaceable properties

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

js_DefineOwnProperty should really be used to implement DefineOwnProperty, using it for replaceable properties fails for proxies (like DOM binding proxies or Xrays) because we end up in some code that expects the PropertyDescriptor to have been created from a JS object that was passed in. The generated code for replaceable properties creates its own PropertyDescriptor, not passed in from JS. JS_DefineProperty works equally well here.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 824029 [details] [diff] [review]
v1

r=me, though I wonder how this came up....  Where do we have replaceable properties on a proxy, exactly?  Note that the Window binding is NOT a proxy.  Was the issue xrays?

Comment 2

[Peter Van der Beken [:peterv]]

Yes, we're hitting this with Xrays. But at one point I accidentally made Window a proxy too, and hit it there.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Given bug 933513 we'll need to backport this to Aurora 27 too, right?

Comment 4

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/392ee11b1ff6

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

either this patch or the patch on bug 932315 caused a large tp5o shutdown regression on windows xp: 
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/r9iehVDynmM

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/392ee11b1ff6

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Joel Maher (:jmaher) from comment #5)
> either this patch or the patch on bug 932315 caused a large tp5o shutdown
> regression on windows xp: 

Backouts on try seem to implicate bug 932315, which is pretty weird. But backing out just this patch didn't change the numbers significantly:

Both backed out: https://tbpl.mozilla.org/?tree=Try&rev=5a846b15ad15
Just this backed out: https://tbpl.mozilla.org/?tree=Try&rev=ccc02d51f76b
Nothing backed out: https://tbpl.mozilla.org/?tree=Try&rev=9571d819903a

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 824029 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 932322
User impact if declined: broken addons and websites
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to Peter Van der Beken [:peterv] from comment #8)
> Comment on attachment 824029 [details] [diff] [review]
> Bug caused by (feature/regressing bug #): bug 932322

Er, that was meant to be bug 918345.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c03363d21c7d