Bug 153245 - [ES6] Fix various issues with TypedArrays.
Summary: [ES6] Fix various issues with TypedArrays.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks: 120112
  Show dependency treegraph
 
Reported: 2016-01-19 12:03 PST by Keith Miller
Modified: 2016-01-21 11:08 PST (History)
5 users (show)

See Also:


Attachments
Patch (16.93 KB, patch)
2016-01-19 13:39 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (16.36 KB, patch)
2016-01-19 16:29 PST, Keith Miller
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-01-19 12:03:11 PST
[ES6] Fix various issues with TypedArrays.
Comment 1 Keith Miller 2016-01-19 13:39:02 PST
Created attachment 269288 [details]
Patch
Comment 2 Keith Miller 2016-01-19 16:29:56 PST
Created attachment 269307 [details]
Patch
Comment 3 Geoffrey Garen 2016-01-19 17:04:00 PST
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?
Comment 4 Keith Miller 2016-01-19 17:07:35 PST
(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.
Comment 5 Keith Miller 2016-01-20 11:28:25 PST
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
Comment 6 Keith Miller 2016-01-20 11:32:43 PST
Committed r195360: <http://trac.webkit.org/changeset/195360>
Comment 7 Darin Adler 2016-01-20 17:24:44 PST
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?
Comment 8 Keith Miller 2016-01-20 20:34:22 PST
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())));
Comment 9 Keith Miller 2016-01-21 10:35:16 PST
(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.