Stop using getter/setter ClassOps in CTypes code

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Posted patch PatchSplinter Review
We want to remove these ClassOps, see bug 1389510.

I'm attaching a patch that wraps array CData objects in a proxy. The proxy forwards most operations to the target object, but it intercepts the get and set traps to implement the indexed property behavior. CData objects no longer have the getProperty and setProperty ClassOps.

This seems to work and it passes the pretty extensive test_jsctypes.js

The proxy's behavior is not perfect: getOwnPropertyNames, getOwnPropertyDescriptor etc don't return the indexed properties. This is similar to the old behavior though, where these properties only showed up after they had been assigned to, so I don't think this matters too much.
Attachment #8896599 - Flags: review?(jorendorff)
Comment on attachment 8896599 [details] [diff] [review]
Patch

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

::: js/src/ctypes/CTypes.cpp
@@ +7757,5 @@
>  
>  bool
>  CData::IsCData(JSObject* obj)
>  {
>    return JS_GetClass(obj) == &sCDataClass;

Can CData::IsCData(JSObject*) assert that the argument isn't an array wrapper? I think it's always a mistake for us to be asking that of the wrapper.

@@ +8072,5 @@
>                                   InformalValueTypeName(args.thisv()));
>    }
>  
>    JSString* result;
> +  if (CData::IsCDataMaybeUnwrap(&obj)) {

It will have been unwrapped already, a few lines earlier.
Attachment #8896599 - Flags: review?(jorendorff) → review+

Comment 3

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6fe814cd66
Use a proxy wrapper object for CTypes arrays instead of getProperty/setProperty Class hooks. r=jorendorff

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c6fe814cd66
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.