Closed Bug 1015798 Opened 6 years ago Closed 3 years ago

Figure out a solution for resolving data properties on XrayToJS prototypes and constructors

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: bholley, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

The typed array initialization code defines the constant BYTES_PER_ELEMENT on both constructors and prototypes for typed array classes. Unfortunately, we don't have a way to represent data properties in JSPropertySpecs.

This constant doesn't seem to be used much in the browser, so I'm just going to stick this work in the FinishClassInitOp and just punt for Xrays. We may need to solve this at some point though.
Fwiw, DOM code ends up making up a ConstantSpec thing for constants like this... I agree that it might be nice if JSPropertySpec could handle it.
In Bug 1299321 I implemented this for string values, should be easy to extend to other value types.
Assignee: nobody → evilpies
Attached patch WIP (obsolete) — Splinter Review
Comment on attachment 8805805 [details] [diff] [review]
WIP

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

::: js/src/vm/TypedArrayObject.cpp
@@ -2625,3 @@
>      nullptr,                                                                   \
> -    _type##Array::finishClassInit,                                             \
> -    JSProto_TypedArray                                                         \

When I don't remove this GlobalObject::resolveConstructor wont define my properties, because StandardClassIsDependent returns true. On the other hand this seems to break Xrays, and .length on typed arrays ends up as undefined ...
Attachment #8805805 - Attachment is obsolete: true
I know this macro stuff is horrible :(. At least introducing PropertySpec::getValue allows us to share code and we don't have to modify XrayWrapper in the future to introduce new types.
Attachment #8807497 - Flags: review?(arai.unmht)
Comment on attachment 8807497 [details] [diff] [review]
Add int32 type to PropertySpec

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +431,5 @@
>              }
>              desc.setAttributes(flags);
>          } else {
> +            RootedValue v(cx);
> +            if (!psMatch->getValue(cx, &v))

you could pass |desc.value()| directly.
(I'm fine with either tho)
Attachment #8807497 - Flags: review?(arai.unmht) → review+
I changed the name of "dependent" to "protoKey", which still isn't great, because the JSClass also has a protokey. I considered naming it baseClass- or inheritance-protoKey maybe?

The actual behavior change is the removal of the StandardClassIsDependent check in GlobalObject::resolveConstructor. With this change we also define properties on object that inherit from something else, like Errors or TypedArrays.
Because of the previous patch we actually define properties for objects with interesting protos.
Attachment #8807975 - Flags: review?(arai.unmht)
This is a bit unfortunate and wrong basically. Currently all our "sub" Error types like TypeError, RangeError inherit from "Error", which is wrong (bug 1213341). So instead of duplicating properties in that case we are supposed to inherit those from Error.prototype.
Attachment #8807976 - Flags: review?(arai.unmht)
Boris, please review the second patch. Thanks!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=189c492645c2
Flags: needinfo?(bzbarsky)
Attachment #8807975 - Flags: review?(arai.unmht) → review+
Attachment #8807976 - Flags: review?(arai.unmht) → review+
Comment on attachment 8807974 [details] [diff] [review]
Improve ClassSpec "depedent" behavior

I'm not super-happy about it not being very clear whether protoKey() returns the protokey of the prototype of the objects that use this classspec or the prototype of that prototype.  Which one is it in practice?

>+    // We're operating on the self-hosting global, in which case we don't want any

This should probably me more like:

  // If we're operating on the self-hosting global, we don't want any ...

r=me
Flags: needinfo?(bzbarsky)
Attachment #8807974 - Flags: review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8ee3aeecac
Add int32 type to PropertySpec. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d09c525ca675
Improve ClassSpec depedent behavior especially with inheritance. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/546dbaaff8e9
Define BYTES_PER_ELEMENT on typed arrays via ClassSpec. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/138f4cfcbd9d
Don't define properties on sub Error types. r=arai
Depends on: 1316829
No longer depends on: 1316829
You need to log in before you can comment on or make changes to this bug.