Closed Bug 1187232 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

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: 4 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!
Depends on: 1248412
You need to log in before you can comment on or make changes to this bug.