Closed Bug 1165722 Opened 8 years ago Closed 8 years ago

Replace JS_GetPropertyDescriptor in Xray code

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
(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.
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.
Attached patch xray-getpropertySplinter Review
Attachment #8614146 - Flags: review?(bobbyholley)
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.
Attachment #8614146 - Flags: review?(bobbyholley) → review+
Assignee: nobody → evilpies
https://hg.mozilla.org/mozilla-central/rev/f25df538837b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.