TypedArrays don't support new named properties

RESOLVED FIXED in mozilla30

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Carlos Scheidegger, Assigned: bhackett)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs, {dev-doc-complete, relnote})

Trunk
mozilla30
dev-doc-complete, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 30+)

Details

(Whiteboard: [js:t][DocArea=JS])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 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.
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?
(Reporter)

Comment 2

6 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.
This should be fixed when the property-element split happens, I suspect.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Depends on: 586842
Bug 827490 just landed.  Does it help?
(Assignee)

Comment 5

4 years ago
Typed array behavior wasn't affected by bug 827490.
Duplicate of this bug: 828599
Blocks: 901723
Not a performance fault in itself, but prevents more-performant implementation of AS3's ByteArray.
Blocks: 885526
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [js:t]
Version: 10 Branch → Trunk

Comment 8

4 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.)
Flags: needinfo?(dherman)
(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

4 years ago
Ah, nice find.  I was basing my understanding off an email thread a few months back.
(Reporter)

Comment 11

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

4 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"?
(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.")
Flags: needinfo?(dherman)
Duplicate of this bug: 901723
Duplicate of this bug: 952403
Blocks: 694100
Blocks: 939927

Comment 17

3 years ago
Can we get an update on when this will be fixed?
(Assignee)

Comment 18

3 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.
(Assignee)

Comment 19

3 years ago
Created attachment 8383094 [details] [diff] [review]
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.
Assignee: general → bhackett1024
(Assignee)

Comment 20

3 years ago
Created attachment 8383104 [details] [diff] [review]
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.
Attachment #8383104 - Flags: review?(jdemooij)
(Assignee)

Comment 21

3 years ago
Created attachment 8383106 [details] [diff] [review]
everything else

Remaining changes to the VM.
Attachment #8383106 - Flags: review?(luke)
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.
Attachment #8383104 - Flags: review?(jdemooij) → review+

Comment 23

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

Comment 24

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

Comment 25

3 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.
(Assignee)

Comment 26

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

Comment 27

3 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.
(Assignee)

Comment 28

3 years ago
Created attachment 8384085 [details] [diff] [review]
updated

This adds IsTypedArrayIndex as requested.
Attachment #8383094 - Attachment is obsolete: true
Attachment #8383104 - Attachment is obsolete: true
Attachment #8383106 - Attachment is obsolete: true
Attachment #8383106 - Flags: review?(luke)
Attachment #8384085 - Flags: review?(luke)
(Assignee)

Updated

3 years ago
Depends on: 978562

Comment 29

3 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.
Attachment #8384085 - Flags: review?(luke) → review+
(Assignee)

Comment 30

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/78fa90a29c43
Keywords: dev-doc-needed
Keywords: relnote

Comment 31

3 years ago
Any reason not to switch the StringIsTypedArrayIndex to uint64?
https://hg.mozilla.org/mozilla-central/rev/78fa90a29c43
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Comment 33

3 years ago
Created attachment 8386119 [details] [diff] [review]
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.
Attachment #8386119 - Flags: review?(luke)

Comment 34

3 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.
Attachment #8386119 - Flags: review?(luke) → review+
(Assignee)

Comment 35

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/25bbf68caed4
Depends on: 980013
Depends on: 980119
Depends on: 980126
https://hg.mozilla.org/mozilla-central/rev/25bbf68caed4

Updated

3 years ago
Duplicate of this bug: 907874
relnote-firefox: --- → ?
relnote-firefox: ? → 30+
Whiteboard: [js:t] → [js:t][DocArea=JS]
Depends on: 986357
QA Whiteboard: [good first verify]
(Assignee)

Updated

3 years ago
Duplicate of this bug: 929146
Depends on: 1061600
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
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1085299
You need to log in before you can comment on or make changes to this bug.