1187232 - ArrayIterator: Access TypedArray's [[ArrayLength]] instead of performing property access

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Test case:
---
var ta = new Int8Array(4);
Object.defineProperty(ta, "length", {value:0});
[...ta];
---

Expected: Returns [ 0, 0, 0, 0 ]
Actual: Returns []

Spec: ES2015, 22.1.5.2.1, step 8.a

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: typed-array-length.diff

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8700109 [details] [diff] [review]
typed-array-length.diff

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

::: js/src/builtin/Array.js
@@ +696,5 @@
>      // The index might not be an integer, so we have to do a generic get here.
>      var index = UnsafeGetReservedSlot(this, ITERATOR_SLOT_NEXT_INDEX);
>      var itemKind = UnsafeGetInt32FromReservedSlot(this, ITERATOR_SLOT_ITEM_KIND);
>      var result = { value: undefined, done: false };
> +    var len = IsTypedArray(a) ? TypedArrayLength(a) : TO_UINT32(a.length);

As I mentioned earlier, IsTypedArray only recognizes typed arrays from the current global/compartment.  It looks to me like it's probably possible to have a typed array from another global here, behind a cross-compartment wrapper.  I think this would happen if you did

  var ArrayIteratorPrototype = Object.getPrototypeOf([].values());
  ArrayIteratorPrototype.next = Object.getPrototypeOf(newGlobal().Array().values()).next;

and then did the test you have written again.

We have a method, IsPossiblyWrappedTypedArray, that's a better predicate.  TypedArrayLength, on the other hand, doesn't have a wrapped version of it.  I think you'll have to construct your own version that works on typed arrays using callFunction(CallTypedArrayMethodIfWrapped, null, a, "IsTypedArray") (I *think* you can call C++-implemented intrinsics this way, but don't quote me on it).

Anyway -- pretty sure this fillip of the change isn't right.  Add a test, and either fix it or prove my reading wrong.  :-)

::: js/src/tests/ecma_6/TypedArray/iterator.js
@@ +5,5 @@
> +var typedArray = new Int8Array(LENGTH);
> +var arrayFromIterator = [...typedArray];
> +assertEq(arrayFromIterator.length, LENGTH);
> +
> +Object.defineProperty(typedArray, "length", {value:0});

A version with a throwing length accessor would be nice to have, as well, to verify not just that we're not using .length, but that we don't even access it at all.

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: typed-array-length.diff

I'm embarrassed to say that I cargo culted this a bit. I was unable to trigger a problem using newGlobal, until I removed:
"    if (!IsObject(this) || !IsArrayIterator(this)) {
        return callFunction(CallArrayIteratorMethodIfWrapped, this,
                            "ArrayIteratorNext");
    }
"

Which is sort of obvious. This goes to show I have a lot of learning to do about compartments, oy. If you have the time/patience, feel like helping me cause the last test to fail so that I can understand what's going on a little more clearly?

Comment 4

[Morgan Phillips [:mrrrgn]]

Attachment: typed-array-length.diff

The new global test does indeed cause a crash without the "IsPossiblyWrappedTypedArray." I added an IsPossiblyWrappedTypedArrayLength intrinsic as well, however, since I didn't use the method you've described in your last review I'm curious if I've again missed a subtle point.

In any case, hopefully this is closer to the mark.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8705709 [details] [diff] [review]
typed-array-length.diff

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

::: js/src/tests/ecma_6/TypedArray/iterator.js
@@ +16,5 @@
> +i8Array = new Int8Array();
> +Object.defineProperty(i8Array, "length", {
> +    get() {
> +        throw TypeError;
> +    }

Add in set as well.

::: js/src/vm/SelfHosting.cpp
@@ +733,5 @@
>      return true;
>  }
>  
>  static bool
> +intrinsic_IsPossiblyWrappedTypedArrayLength(JSContext* cx, unsigned argc, Value* vp)

Remove "Is" in this name.  This method's always called with a typed array.  And we'd throw an exception if weird GC behavior caused the argument ever to not be a possibly-wrapped typed array.

@@ +739,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() == 1);
> +
> +    int32_t typedArrayLength = 0;
> +    if (args[0].isObject()) {

It looks like this is always an object per its one caller, so just assert this at the top of the method after the length-1 assert.  Also, please make |typedArrayLength| a uint32_t and pass AssertedCast<int32_t>(typedArrayLength) to setInt32.

@@ +1630,5 @@
>      JS_FN("TypedArrayElementShift",  intrinsic_TypedArrayElementShift,  1,0),
>  
>      JS_INLINABLE_FN("TypedArrayLength", intrinsic_TypedArrayLength,     1,0,
>                      IntrinsicTypedArrayLength),
> +    JS_FN("IsPossiblyWrappedTypedArrayLength", intrinsic_IsPossiblyWrappedTypedArrayLength, 1, 0),

Hmm.  Good enough to land, but I think you should do a quick followup (or second patch in this bug, that can be separately landed) to make this inlinable/inline this, when the argument is known by TI to be a typed array.  Take a gander at jit/MCallOptimize.cpp's typed array methods and how they're inlined -- this'll be pretty easy to implement, and a good opportunity to start dabbling in JIT-land.

Comment 6

[Jeff Walden [:Waldo]]

I should also say: adding a new intrinsic is a perfectly fine alternative to doing the callFunction thing I posited in comment 2.  And probably a better one, once you consider an intrinsic would be inlinable (and clearer), where the callFunction approach approximately isn't.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc1687025ec

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fc1687025ec

Comment 9

[Guilherme Lima]

Apparently this caused 100% regression on misc-basic-array-forof on AWFY. But still faster than Safari and Chrome.

Comment 10

[Till Schneidereit [:till]]

(In reply to Guilherme Lima from comment #9)
> Apparently this caused 100% regression on misc-basic-array-forof on AWFY.
> But still faster than Safari and Chrome.

Probably bug 1239068.

Comment 11

[Morgan Phillips [:mrrrgn]]

(In reply to Guilherme Lima from comment #9)
> Apparently this caused 100% regression on misc-basic-array-forof on AWFY.
> But still faster than Safari and Chrome.

Till hit the nail on the head, I've got a patch with it inlined ready to go, that should fix up the issue.

Comment 12

[Guilherme Lima]

(In reply to Morgan Phillips [:mrrrgn] from comment #11)
> (In reply to Guilherme Lima from comment #9)
> > Apparently this caused 100% regression on misc-basic-array-forof on AWFY.
> > But still faster than Safari and Chrome.
> 
> Till hit the nail on the head, I've got a patch with it inlined ready to go,
> that should fix up the issue.

Unfortunately that patch didn't fix the regression.

Comment 13

[Morgan Phillips [:mrrrgn]]

(In reply to Guilherme Lima from comment #12)
> (In reply to Morgan Phillips [:mrrrgn] from comment #11)
> > (In reply to Guilherme Lima from comment #9)
> > > Apparently this caused 100% regression on misc-basic-array-forof on AWFY.
> > > But still faster than Safari and Chrome.
> > 
> > Till hit the nail on the head, I've got a patch with it inlined ready to go,
> > that should fix up the issue.
> 
> Unfortunately that patch didn't fix the regression.

Hmm, apologies, that's odd. I'll investigate this later. Thank you!