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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Tom S [:evilpie]]

In Bug 1299321 I implemented this for string values, should be easy to extend to other value types.

Comment 3

[Tom S [:evilpie]]

Attachment: WIP

Comment 4

[Tom S [:evilpie]]

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 ...

Comment 5

[Tom S [:evilpie]]

Attachment: Add int32 type to PropertySpec

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.

Comment 6

[Tooru Fujisawa [:arai]]

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)

Comment 7

[Tom S [:evilpie]]

Attachment: Improve ClassSpec "depedent" behavior

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.

Comment 8

[Tom S [:evilpie]]

Attachment: Define BYTES_PER_ELEMENT on typed arrays via ClassSpec

Because of the previous patch we actually define properties for objects with interesting protos.

Comment 9

[Tom S [:evilpie]]

Attachment: Don't define properties on "sub" Error types

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.

Comment 10

[Tom S [:evilpie]]

Boris, please review the second patch. Thanks!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=189c492645c2

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec8ee3aeecac
https://hg.mozilla.org/mozilla-central/rev/d09c525ca675
https://hg.mozilla.org/mozilla-central/rev/546dbaaff8e9
https://hg.mozilla.org/mozilla-central/rev/138f4cfcbd9d