Carlos Scheidegger Reporter
	Description • 13 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 13 years ago

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?

	Carlos Scheidegger Reporter
	Comment 2 • 13 years ago

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

	Jeff Walden [:Waldo]
	Comment 3 • 13 years ago

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

	Nicholas Nethercote [inactive]
	Comment 4 • 11 years ago

Bug 827490 just landed.  Does it help?

	Brian Hackett [Laid off!] Assignee
	Comment 5 • 11 years ago

Typed array behavior wasn't affected by bug 827490.

	Till Schneidereit [:till]
	Comment 7 • 11 years ago

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

	Luke Wagner [:luke]
	Comment 8 • 11 years ago

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

	Till Schneidereit [:till]
	Comment 9 • 11 years ago

(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

	Luke Wagner [:luke]
	Comment 10 • 11 years ago

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

	Carlos Scheidegger Reporter
	Comment 11 • 11 years ago

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?

	Till Schneidereit [:till]
	Comment 12 • 11 years ago

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.

	rudebar
	Comment 13 • 11 years ago

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"?

	Till Schneidereit [:till]
	Comment 14 • 11 years ago

(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.")

	Feross Aboukhadijeh
	Comment 17 • 10 years ago

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

	Brian Hackett [Laid off!] Assignee
	Comment 18 • 10 years ago

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

	Brian Hackett [Laid off!] Assignee
	Comment 19 • 10 years ago

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.

	Brian Hackett [Laid off!] Assignee
	Comment 20 • 10 years ago

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

	Brian Hackett [Laid off!] Assignee
	Comment 21 • 10 years ago

Remaining changes to the VM.

	Jan de Mooij [:jandem]
	Comment 22 • 10 years ago

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.

	Luke Wagner [:luke]
	Comment 23 • 10 years ago

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.

	Luke Wagner [:luke]
	Comment 24 • 10 years ago

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?

	André Bargull [:anba]
	Comment 25 • 10 years ago

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

	Brian Hackett [Laid off!] Assignee
	Comment 26 • 10 years ago

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

	Luke Wagner [:luke]
	Comment 27 • 10 years ago

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.

	Brian Hackett [Laid off!] Assignee
	Comment 28 • 10 years ago

This adds IsTypedArrayIndex as requested.

	Luke Wagner [:luke]
	Comment 29 • 10 years ago

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.

	Brian Hackett [Laid off!] Assignee
	Comment 30 • 10 years ago

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

	Luke Wagner [:luke]
	Comment 31 • 10 years ago

Any reason not to switch the StringIsTypedArrayIndex to uint64?

	Carsten Book [:Tomcat]
	Comment 32 • 10 years ago

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

	Brian Hackett [Laid off!] Assignee
	Comment 33 • 10 years ago

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.

	Luke Wagner [:luke]
	Comment 34 • 10 years ago

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.

	Brian Hackett [Laid off!] Assignee
	Comment 35 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 36 • 10 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 39 • 10 years ago

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