1165722 - Replace JS_GetPropertyDescriptor in Xray code

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Tom S [:evilpie]]

Normally I would assume that holders only have own properties, but at least this comment claims otherwise.

// The generic Xray machinery only defines non-own properties on the holder.
// This is broken, and will be fixed at some point, but for now we need to
// cache the value explicitly.

We need to fix this now to use JS_GetOwnPropertyDescriptor

Comment 1

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #0)
> Normally I would assume that holders only have own properties, but at least
> this comment claims otherwise.
> 
> // The generic Xray machinery only defines non-own properties on the holder.
> // This is broken, and will be fixed at some point, but for now we need to
> // cache the value explicitly.
> 
> We need to fix this now to use JS_GetOwnPropertyDescriptor

You're misinterpreting the comment, I think. The holder is a cache (with a null proto), which only has own properties - it's just a question of what properties of the target object (own vs non-own) should be cached. I think the usage you're referring to could be replaced with JS_GetOwnPropertyDescriptor without any behavior change.

Comment 2

[Boris Zbarsky [:bzbarsky]]

We should clarify the comment, though.  Perhaps like this:

  // The generic Xray machinery only defines non-own properties of the target on
  // the holder.  This is broken, and will be fixed at some point, but for now we
  // need to cache the value explicitly.

Comment 3

[Tom S [:evilpie]]

Attachment: xray-getproperty

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8614146 [details] [diff] [review]
xray-getproperty

Review of attachment 8614146 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ -523,5 @@
>  
> -        // The generic Xray machinery only defines non-own properties on the holder.
> -        // This is broken, and will be fixed at some point, but for now we need to
> -        // cache the value explicitly. See the corresponding call to
> -        // JS_GetPropertyById at the top of this function.

Please reinstate and adjust this comment appropriately.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f25df538837b

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f25df538837b