Closed Bug 1187232 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox42 --- affected
firefox46 --- fixed

People

(Reporter: anba, Assigned: mrrrgn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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
Assignee: nobody → winter2718
Attached patch typed-array-length.diff (obsolete) — Splinter Review
Attachment #8700109 - Flags: review?(jwalden+bmo)
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.
Attachment #8700109 - Flags: review?(jwalden+bmo) → review-
Attached patch typed-array-length.diff (obsolete) — Splinter Review
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?
Attachment #8700109 - Attachment is obsolete: true
Attachment #8701630 - Flags: feedback?(jwalden+bmo)
Attachment #8701630 - Flags: feedback?(jwalden+bmo)
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.
Attachment #8701630 - Attachment is obsolete: true
Attachment #8705709 - Flags: review?(jwalden+bmo)
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.
Attachment #8705709 - Flags: review?(jwalden+bmo) → review+
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.
Blocks: 1239068
https://hg.mozilla.org/mozilla-central/rev/7fc1687025ec
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Apparently this caused 100% regression on misc-basic-array-forof on AWFY. But still faster than Safari and Chrome.
(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.
(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.
(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.
(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!
You need to log in before you can comment on or make changes to this bug.