Bug 153245 – [ES6] Fix various issues with TypedArrays.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

[ES6] Fix various issues with TypedArrays.

[Keith Miller]

Created attachment 269288 [details]
Patch

[Keith Miller]

Created attachment 269307 [details]
Patch

[Geoffrey Garen]

Comment on attachment 269307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269307&action=review

r=me

Can you add a test for .byteOffset and .buffer?

> Source/JavaScriptCore/runtime/JSArrayBufferView.h:162
> +    bool isNeutered() { return hasArrayBuffer() && !vector(); }

What prevents FastTypedArray and OversizeTypedArray from being neutered?

[Keith Miller]

(In reply to comment #3)
> Comment on attachment 269307 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269307&action=review
> 
> r=me
> 
> Can you add a test for .byteOffset and .buffer?

.byteOffset is covered by the test I added and I'll add a test for .buffer.

> 
> > Source/JavaScriptCore/runtime/JSArrayBufferView.h:162
> > +    bool isNeutered() { return hasArrayBuffer() && !vector(); }
> 
> What prevents FastTypedArray and OversizeTypedArray from being neutered?

JSArrayBufferViews can only be neutered from their underlying ArrayBuffer. Since
those modes don't have ArrayBuffers they cannot be neutered.

[Keith Miller]

I think .buffer still has some minor issues. I'll put the changes in a different patch: https://bugs.webkit.org/show_bug.cgi?id=153281

[Keith Miller]

Committed r195360: <http://trac.webkit.org/changeset/195360>

[Darin Adler]

Comment on attachment 269307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269307&action=review

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:78
> -        offset = exec->uncheckedArgument(1).toUInt32(exec);
> +        double offsetNumber = exec->uncheckedArgument(1).toInteger(exec);
>          if (exec->hadException())
>              return JSValue::encode(jsUndefined());
> +        if (offsetNumber < 0)
> +            return throwVMRangeError(exec, "Offset should not be negative");
> +        offset = offsetNumber;

This seems like a change in behavior. Will this do the right thing for numbers larger than 2^31? Do we have test cases covering that?

[Keith Miller]

Comment on attachment 269307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269307&action=review

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:78
>> +        offset = offsetNumber;
> 
> This seems like a change in behavior. Will this do the right thing for numbers larger than 2^31? Do we have test cases covering that?

It does change behavior but that's intentional as the spec requires the change see: http://www.ecma-international.org/ecma-262/6.0/#sec-%typedarray%.prototype.set-array-offset. I'm not sure if we have a test for numbers >= 2^31, I will add one. Although, looking at this again, offset = offsetNumber will produce undefined behavior if offsetNumber >= 2^32. I thought the assignment would just round down to the nearest unsigned number, which is not the case. It should be:

offset = static_cast<unsigned>(std::min(offsetNumber, static_cast<double>(std::numeric_limits<unsigned>::max())));

[Keith Miller]

(In reply to comment #8)
> Comment on attachment 269307 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269307&action=review
> 
> >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:78
> >> +        offset = offsetNumber;
> > 
> > This seems like a change in behavior. Will this do the right thing for numbers larger than 2^31? Do we have test cases covering that?
> 
> It does change behavior but that's intentional as the spec requires the
> change see:
> http://www.ecma-international.org/ecma-262/6.0/#sec-%typedarray%.prototype.
> set-array-offset. I'm not sure if we have a test for numbers >= 2^31, I will
> add one. Although, looking at this again, offset = offsetNumber will produce
> undefined behavior if offsetNumber >= 2^32. I thought the assignment would
> just round down to the nearest unsigned number, which is not the case. It
> should be:
> 
> offset = static_cast<unsigned>(std::min(offsetNumber,
> static_cast<double>(std::numeric_limits<unsigned>::max())));

This should be fixed when https://bugs.webkit.org/show_bug.cgi?id=153309 lands.