695438 - TypedArrays don't support new named properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Carlos Scheidegger]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111018 Firefox/10.0a1
Build ID: 20111018031016

Steps to reproduce:

I evaluated the following snippet

(x = new Float32Array([1,2,3,4]), x._foo = "bar", x._foo === "bar")



Actual results:

Its value was false.


Expected results:

Its value should be true, at least according to my reading of the TypedArrays (draft) spec: http://www.khronos.org/registry/typedarray/specs/latest/ 

and indirectly 

http://dev.w3.org/2006/webapi/WebIDL/

Incidentally, Both Safari 5 and Chrome 14, on OS X and 64-bit Ubuntu, evaluate the above expression to true.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I believe that Andreas did this quite purposefully back when he was implementing typed arrays...

What should the behavior be for out-of-range indexed properties?

Comment 2

[Carlos Scheidegger]

... I see.

For completeness, Chrome and Safari both appear to ignore out-of-range indexed property assignments (as does Firefox).

The argument is then whether it is more consistent to ignore all property assignments besides in-range indices, or to only ignore out-of-range property assignments and read-only properties. I believe the latter is closer in spirit to current practice in Javascript.

Comment 3

[Jeff Walden [:Waldo]]

This should be fixed when the property-element split happens, I suspect.

Comment 4

[Nicholas Nethercote [inactive]]

Bug 827490 just landed.  Does it help?

Comment 5

[Brian Hackett [Laid off!]]

Typed array behavior wasn't affected by bug 827490.

Comment 7

[Till Schneidereit [:till]]

Not a performance fault in itself, but prevents more-performant implementation of AS3's ByteArray.

Comment 8

[Luke Wagner [:luke]]

Recent changes to the typed array spec do specify supporting named own properties (while ignoring out-of-range indexed properties), IIRC.  Is that rigth dherman?  (I thought there was a separate bug on this bug I couldn't find one.)

Comment 9

[Till Schneidereit [:till]]

(In reply to Luke Wagner [:luke] from comment #8)
> Recent changes to the typed array spec do specify supporting named own
> properties (while ignoring out-of-range indexed properties), IIRC.  Is that
> rigth dherman?  (I thought there was a separate bug on this bug I couldn't
> find one.)

Looks like TC39 went the other direction:
https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-07/july-24.md#54-are-typedarray-insances-born-non-extensible

Comment 10

[Luke Wagner [:luke]]

Ah, nice find.  I was basing my understanding off an email thread a few months back.

Comment 11

[Carlos Scheidegger]

So if TC39 is considered authoritative, then should the typed array spec be changed to reflect this? Or is my understanding of the current typed array spec incorrect, and it already states that own named properties are disallowed?

Firefox 23 still behaves differently from other browsers (tested Chrome 28.0.1500.95 and Safari 6.0.3 (8536.28.10) all on OS X 10.8.3) in this respect. Ideally the spec would be unambiguous, right?

Comment 12

[Till Schneidereit [:till]]

It is my understanding that the TypedArray spec is going to move into the es262 spec. That would indeed make TC39 the authoritative standards body. AFAICT, there were no representatives from JSC and V8 in the room, which is a bit unfortunate.

I would needinfo dherman about that, but he's needinfo'd already.

Comment 13

[rudebar]

Why should TypedArray's not be extensible? I saw the comments in https://github.com/rwaldron/tc39-notes/blob/master/es6/2013-07/july-24.md#54-are-typedarray-insances-born-non-extensible but this seems to be questionable to me according to the generalized approach of extensible prototypes. There are some real world use-cases that I can provide if you want. Is there any chance that this can be "fixed"?

Comment 14

[Till Schneidereit [:till]]

(In reply to rudebar from comment #13)
> Is there any chance that this can be "fixed"?

TC39 again changed their minds, so typed arrays will be extensible after all: http://esdiscuss.org/topic/extensible-typed-arrays-use-case-in-the-wild

(Niko's reply didn't make it to the list, but it was "Indeed. AFAIK this work has not yet begun but it will be done.")

Comment 17

[Feross Aboukhadijeh]

Can we get an update on when this will be fixed?

Comment 18

[Brian Hackett [Laid off!]]

I will start today on fixing this, it shouldn't take long.  I'm sorry this has been left to sit for so long.

Comment 19

[Brian Hackett [Laid off!]]

Attachment: patch (465f58380970)

This patch allows typed arrays to have own properties like other objects.  Writes to out of bounds indexes are ignored (though writes to negative integers add new properties as usual).

Internally this works by making typed arrays native objects, and handling them in many of the places which currently handle dense elements.  This seemed preferable to adding hooks to the typed array classes to get the desired behavior, as (a) the hook semantics don't quite fit in cases like ignoring new indexed properties, (b) this is easier for the JITs to handle when optimizing, and (c) array lengths are already special cased in many of the same places anyways.

Comment 20

[Brian Hackett [Laid off!]]

Attachment: jit parts

JIT changes.  Most of this is handling the new case in StoreTypedArrayElementHole where an OOL call needs to be made for negative indexes.

Comment 21

[Brian Hackett [Laid off!]]

Attachment: everything else

Remaining changes to the VM.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8383104 [details] [diff] [review]
jit parts

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

Cool! Also nice that they are native objects now.

::: js/src/jit/CodeGenerator.cpp
@@ +7414,5 @@
> +  public:
> +    OutOfLineStoreTypedArrayElementHole(LStoreTypedArrayElementHole *ins)
> +      : ins_(ins)
> +    {
> +        JS_ASSERT(ins->isStoreTypedArrayElementHole());

Nit: you can remove this assert.

::: js/src/jit/IonCaches.cpp
@@ +3954,5 @@
>      if (!GetObjectElementOperationPure(cx, obj, idval, vp.address()))
>          return false;
>  
> +    if (!vp.isInt32())
> +        GetObjectElementOperationPure(cx, obj, idval, vp.address());

This should be removed.

Comment 23

[Luke Wagner [:luke]]

Glad to see this fixed!  One high-level comment: according to the ES6 spec the correct value for negative integers is to unconditionally returned undefined (not consult the prototype chain and definitely not to add named properties).  If you can fix that here, we can dup bug 907874 to this bug.

Comment 24

[Luke Wagner [:luke]]

Comment on attachment 8383106 [details] [diff] [review]
everything else

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

This looks mostly good, just have some questions below and the issue in comment 23.

::: js/src/jit-test/tests/asm.js/testZOOB.js
@@ +102,5 @@
>          var v = +arr[index];
>          arr[index] = 0;
>          f(i, v);
> +        if (index >= 0)
> +            assertEq(+arr[index], v);

From comment 23, you can revert the changes in this file.

::: js/src/jit-test/tests/basic/bug829821.js
@@ +1,1 @@
> +// |jit-test| error:TypeError

Looking at http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.push, push is generic and so I don't see where a type error would be thrown.

::: js/src/jit-test/tests/basic/typed-array-sealed-frozen.js
@@ +63,5 @@
> +  assertEq(a[20], undefined);
> +
> +  // Watch for especially large indexed properties.
> +  a[-10 >>> 0] = "twelve";
> +  assertEq(a[-10 >>> 0], undefined);

Another corner case to check for: since the predicate for "is this an indexed property" in http://people.mozilla.org/~jorendorff/es6-draft.html#sec-8.4.6.4 is ToString(ToInteger(p)) == p, super-large (>2^53) integers (or super-small) will not be considered indices and thus will be named, own properties.  Goofiness, but probably worth a test.

::: js/src/jit-test/tests/collections/Array-of-generic-3.js
@@ +2,1 @@
>  // Array.of can be transplanted to builtin constructors.

Array.of also seems to be generic (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.of) so I'm not sure why we'd get a type error here.

::: js/src/jsobj.cpp
@@ +3818,5 @@
>          if (shape) {
> +            if (IsImplicitDenseOrTypedArrayElement(shape)) {
> +                if (obj->is<TypedArrayObject>()) {
> +                    /* Ignore getter/setter properties added to typed arrays. */
> +                    return true;

I don't understand this: shouldn't it be possible to define a getter/setter own property on a typed array so long as the property name isn't an index?

@@ +3970,5 @@
>                                   typename MaybeRooted<Shape*, allowGC>::MutableHandleType propp,
>                                   bool *donep)
>  {
> +    uint32_t index;
> +    if (js_IdIsIndex(id, &index)) {

So, based on the definition in
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-integer-indexed-exotic-objects-get-p-receiver
we'll probably need two predicates: one for the internal optimization of dense arrays (js_IdIsIndex) and one for the spec-defined (ToString(ToInteger(p)) == p) condition (say, IsTypedArrayIndex).

@@ +5123,5 @@
> +    if (nobj->isNative() && IsImplicitDenseOrTypedArrayElement(shape)) {
> +        if (nobj->is<TypedArrayObject>()) {
> +            if (*attrsp == (JSPROP_ENUMERATE | JSPROP_PERMANENT))
> +                return true;
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_SET_ARRAY_ATTRS);

Similarly, I would think this logic would only apply when the id is a typed-array index; own named properties should be able to do whatever.

@@ +5161,5 @@
>  
> +    if (IsImplicitDenseOrTypedArrayElement(shape)) {
> +        if (obj->is<TypedArrayObject>()) {
> +            // Don't delete elements from typed arrays.
> +            *succeeded = false;

Same here.

::: js/src/vm/ObjectImpl.cpp
@@ +159,5 @@
> +ObjectImpl::canHaveNonEmptyElements()
> +{
> +    if (isNative())
> +        return !static_cast<JSObject *>(this)->is<TypedArrayObject>();
> +    return static_cast<JSObject *>(this)->is<ArrayBufferObject>();

Given that typed arrays will always have null 'elements', do you, in a later patch, we'll be able to reclaim the the elements word (and perhaps the slots word when there are no named own properties) for inline typed array storage?

Comment 25

[André Bargull [:anba]]

(In reply to Luke Wagner [:luke] from comment #24)
> ::: js/src/jit-test/tests/basic/bug829821.js
> @@ +1,1 @@
> > +// |jit-test| error:TypeError
> 
> Looking at
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.
> push, push is generic and so I don't see where a type error would be thrown.

Step 8 in Array.prototype.push fails with a TypeError, because "length" is not writable for typed arrays and Put() is called with throw=true.


> 
> ::: js/src/jit-test/tests/basic/typed-array-sealed-frozen.js
> @@ +63,5 @@
> > +  assertEq(a[20], undefined);
> > +
> > +  // Watch for especially large indexed properties.
> > +  a[-10 >>> 0] = "twelve";
> > +  assertEq(a[-10 >>> 0], undefined);
> 
> Another corner case to check for: since the predicate for "is this an
> indexed property" in
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-8.4.6.4 is
> ToString(ToInteger(p)) == p, super-large (>2^53) integers (or super-small)
> will not be considered indices and thus will be named, own properties. 
> Goofiness, but probably worth a test.

Note that the current definition for indexed properties is not yet correct, cf. https://bugs.ecmascript.org/show_bug.cgi?id=2049 .


> 
> ::: js/src/jit-test/tests/collections/Array-of-generic-3.js
> @@ +2,1 @@
> >  // Array.of can be transplanted to builtin constructors.
> 
> Array.of also seems to be generic
> (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.of) so I'm
> not sure why we'd get a type error here.

Array.of() creates properties with configurable=true, but indexed properties on typed arrays are configurable=false, cf. CreateDataPropertyOrThrow() in step 8c.

Comment 26

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #24)
> ::: js/src/jsobj.cpp
> @@ +3818,5 @@
> >          if (shape) {
> > +            if (IsImplicitDenseOrTypedArrayElement(shape)) {
> > +                if (obj->is<TypedArrayObject>()) {
> > +                    /* Ignore getter/setter properties added to typed arrays. */
> > +                    return true;
> 
> I don't understand this: shouldn't it be possible to define a getter/setter
> own property on a typed array so long as the property name isn't an index?
> 
> @@ +5123,5 @@
> > +    if (nobj->isNative() && IsImplicitDenseOrTypedArrayElement(shape)) {
> > +        if (nobj->is<TypedArrayObject>()) {
> > +            if (*attrsp == (JSPROP_ENUMERATE | JSPROP_PERMANENT))
> > +                return true;
> > +            JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_SET_ARRAY_ATTRS);
> 
> Similarly, I would think this logic would only apply when the id is a
> typed-array index; own named properties should be able to do whatever.
> 
> @@ +5161,5 @@
> >  
> > +    if (IsImplicitDenseOrTypedArrayElement(shape)) {
> > +        if (obj->is<TypedArrayObject>()) {
> > +            // Don't delete elements from typed arrays.
> > +            *succeeded = false;
> 
> Same here.

All of these depend on IsImplicitDenseOrTypedArrayElement holding, which means the lookup found an element of the typed array, and not a named property.

> ::: js/src/vm/ObjectImpl.cpp
> @@ +159,5 @@
> > +ObjectImpl::canHaveNonEmptyElements()
> > +{
> > +    if (isNative())
> > +        return !static_cast<JSObject *>(this)->is<TypedArrayObject>();
> > +    return static_cast<JSObject *>(this)->is<ArrayBufferObject>();
> 
> Given that typed arrays will always have null 'elements', do you, in a later
> patch, we'll be able to reclaim the the elements word (and perhaps the slots
> word when there are no named own properties) for inline typed array storage?

I doubt it, though maybe that will be able to change in the future.  A typed array's |elements| is actually emptyObjectElements and there are still places we will inspect this.  These could be fixed but I don't think the gain realized will be all that big because typed array instances are pretty bloated.

Comment 27

[Luke Wagner [:luke]]

Ah, thanks for explaining Andre.  So that nixes all the comments about the generic methods.  So with that and Brian's other answer, my only request is that, even if the definition of what exactly is a typed array index is going to be further tweaked, that it be separated out into a new predicate other than js_IdIsIndex() and that this predicate say "yes" for negative integers.  The exact set of strings and large doubles accepted may change, bit iiuc, this much won't.

Comment 28

[Brian Hackett [Laid off!]]

Attachment: updated

This adds IsTypedArrayIndex as requested.

Comment 29

[Luke Wagner [:luke]]

Comment on attachment 8384085 [details] [diff] [review]
updated

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

Great, thanks!

::: js/src/jit-test/tests/basic/typed-array-sealed-frozen.js
@@ +67,5 @@
> +  assertEq(a[-10 >>> 0], undefined);
> +
> +  // Watch for overly large indexed properties.
> +  a[Math.pow(2, 53)] = "twelve";
> +  assertEq(a[Math.pow(2, 53)], "twelve");

Could you also add tests for deleting and defineProperty'ing own properties of typed arrays?

::: js/src/vm/TypedArrayObject.cpp
@@ +2475,5 @@
>      return v.isObject() && v.toObject().is<ArrayBufferObject>();
>  }
>  
> +bool
> +js::StringIsTypedArrayIndex(JSLinearString *str, double *indexp)

It seems like you the outparam type could be uint64_t here and always use UINT64_MAX as the "is an index, but definitely out of range" value.  That avoids the interface ambiguity inherent in returning a double.

@@ +2524,5 @@
> +        index = 10 * index + digit;
> +    }
> +
> +    if (negative)
> +        index = -index;

With a uint64_t, you'd just have 'index = UINT64_MAX' since negative is unconditionally out-of-bounds.

Comment 30

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78fa90a29c43

Comment 31

[Luke Wagner [:luke]]

Any reason not to switch the StringIsTypedArrayIndex to uint64?

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78fa90a29c43

Comment 33

[Brian Hackett [Laid off!]]

Attachment: tweak StringIsTypedArrayIndex

Well, it seemed kind of bizarre to compute a uint64_t index whose representation is constrained by the limits of a double mantissa.  But now I'm seeing people talking about any integer string as being an index and I'm pretty confused about what exactly the semantics this function is supposed to be doing are.  The attached patch does the latter and uses a uint64_t outparam.  Note that to land the previous patch I disallowed "Infinity" and "-Infinity" because CharsEqualAscii was causing nonsense assertion failures on Linux and because treating "Infinity" as an integer is pretty strange.

Comment 34

[Luke Wagner [:luke]]

Comment on attachment 8386119 [details] [diff] [review]
tweak StringIsTypedArrayIndex

> But now I'm seeing people talking about any integer string as being an 
> index and I'm pretty confused about what exactly the semantics this 
> function is supposed to be doing are.

Probably because the exact specification on these edge cases is currently in flux :)  I'm sure all engines are currently wildly different on the corner cases here, but I don't think it matters too much until we have something more final in the spec.  The important part from my perspective in this bug was just having this predicate factored out and using uint64_t to abstract from the final details and to keep the interface simple.

The latest draft of spec wording I saw from Allen is a bit different, but it may again change in the future when it gets discussed at the next meeting, so this seems like as good a first approximation as any.

Comment 35

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25bbf68caed4

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/25bbf68caed4

Comment 39

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/30#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray#Property_access