1122396 - Make %TypedArray%.of and %TypedArray%.from non-generic methods

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Comment 1

[André Bargull [:anba]]

js> Int8Array.from.call(function(){}, [])

Expected: Throws TypeError
Actual: no error


js> Int8Array.of.call(function(){})

Expected: Throws TypeError
Actual: no error

Comment 2

[André Bargull [:anba]]

Also applies to:

22.2.3.9 %TypedArray%.prototype.filter
22.2.3.18 %TypedArray%.prototype.map
22.2.3.23 %TypedArray%.prototype.slice

Comment 3

[Morgan Phillips [:mrrrgn]]

We've since implemented this check for all of the typed array methods with a note like 

"This function is not generic. The this value must be an object with a [[TypedArrayName]] internal slot."

Including filter, map, slice, etc...

%TypedArray%.from and %TypedArray%.of have no such note however, and only explicitly ask for an "IsConstructor" check. My reading of the spec leads me to believe that these are generic, like their Array counterparts.

Asking for a second opinion before closing bug.

Comment 4

[Morgan Phillips [:mrrrgn]]

Okay, the problem here is that we've not properly implemented `22.2.4.6 TypedArrayCreate` , the use of this function implies that from/of are indeed not generic. Apologies for the previous comment.

Comment 5

[Morgan Phillips [:mrrrgn]]

Attachment: typedarray-from.diff

So, it seems that %TypedArray%.from and of are indeed generic methods with the caveat that they must return validated typed arrays. 

So a `this` which isn't a TypedArray is acceptable so long as constructing it returns a TypedArray with the same length as the list of items passed to from/of. This is reflected in the included tests. see: ecma_6/TypedArray/from_constructor.js

Comment 6

[André Bargull [:anba]]

Attachment: bug1122396.patch

New patch, applies on top of bug 1309701.

Comment 7

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5053c041d3dc795ba93e754038ee954e45c36e94

Comment 8

[Tom S]

Comment on attachment 8800410 [details] [diff] [review]
bug1122396.patch

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

+1 Incredible extensive tests.

::: js/src/builtin/TypedArray.js
@@ +63,5 @@
>  
> +// ES2017 draft rev 6859bb9ccaea9c6ede81d71e5320e3833b92cb3e
> +// 22.2.3.5.1 Runtime Semantics: ValidateTypedArray ( O )
> +function ValidateTypedArray(obj, error) {
> +    if (IsObject(obj)) {

Is there some perf improvement by avoiding the early return here?

@@ +66,5 @@
> +function ValidateTypedArray(obj, error) {
> +    if (IsObject(obj)) {
> +        /* Steps 3-5 (non-wrapped typed arrays). */
> +        if (IsTypedArray(obj)) {
> +            GetAttachedArrayBuffer(obj);

// GetAttachedArrayBuffer throws for detached array buffers

::: js/src/tests/ecma_6/TypedArray/from_string.js
@@ +1,1 @@
>  // %TypedArray%.from called on Array should also handle strings correctly.

I am not sure how much value this test provides now.

::: js/src/tests/ecma_7/TypedArray/filter-validation.js
@@ +6,5 @@
> +
> +const otherGlobal = typeof newGlobal === "function" ? newGlobal() : undefined;
> +
> +// Throws TypeError when the returned value is not a typed array.
> +for (const TAConstructor of anyTypedArrayConstructors) {

I think you could have minimized those tests by having two nested loops, the outer loop with the typed array types, and the inner loop with the constructor function and the expected TypeError.

::: js/src/vm/SelfHosting.cpp
@@ +1184,5 @@
> +    MOZ_ASSERT(args.length() == 1);
> +    MOZ_ASSERT(args[0].isObject());
> +
> +    JSObject* obj = CheckedUnwrap(&args[0].toObject());
> +

Delete blank line

Comment 9

[André Bargull [:anba]]

Attachment: bug1122396.patch

Updated patch per review comments, carrying r+ from evilpie.

Comment 10

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #8)
> Comment on attachment 8800410 [details] [diff] [review]
> bug1122396.patch
> 
> Review of attachment 8800410 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> +1 Incredible extensive tests.

^^


> ::: js/src/builtin/TypedArray.js
> @@ +63,5 @@
> >  
> > +// ES2017 draft rev 6859bb9ccaea9c6ede81d71e5320e3833b92cb3e
> > +// 22.2.3.5.1 Runtime Semantics: ValidateTypedArray ( O )
> > +function ValidateTypedArray(obj, error) {
> > +    if (IsObject(obj)) {
> 
> Is there some perf improvement by avoiding the early return here?

No, I've just did this way to avoid calling ThrowTypeError twice (for the non-Object case and for the non-TypedArray case).


> 
> @@ +66,5 @@
> > +function ValidateTypedArray(obj, error) {
> > +    if (IsObject(obj)) {
> > +        /* Steps 3-5 (non-wrapped typed arrays). */
> > +        if (IsTypedArray(obj)) {
> > +            GetAttachedArrayBuffer(obj);
> 
> // GetAttachedArrayBuffer throws for detached array buffers
> 
> ::: js/src/tests/ecma_6/TypedArray/from_string.js
> @@ +1,1 @@
> >  // %TypedArray%.from called on Array should also handle strings correctly.
> 
> I am not sure how much value this test provides now.

Well, at least one test to ensure TypedArray.from works when iterating over a String. (I didn't remove it in the updated patch.)


> 
> ::: js/src/tests/ecma_7/TypedArray/filter-validation.js
> @@ +6,5 @@
> > +
> > +const otherGlobal = typeof newGlobal === "function" ? newGlobal() : undefined;
> > +
> > +// Throws TypeError when the returned value is not a typed array.
> > +for (const TAConstructor of anyTypedArrayConstructors) {
> 
> I think you could have minimized those tests by having two nested loops, the
> outer loop with the typed array types, and the inner loop with the
> constructor function and the expected TypeError.

Yes, much better now with all those duplicates lines removed!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed543ea2c0ad
Validate newly created instances in typed array builtins. r=evilpie

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ed543ea2c0ad

Comment 13

[Florian Scholz (Open Web Docs)]

Added to developer release notes https://developer.mozilla.org/en-US/Firefox/Releases/52#JavaScript

This could add two new error pages linked from the console, if we want to further explain this. I'm not feeling strongly about it for now.

JSMSG_NON_TYPED_ARRAY_RETURNED: constructor didn't return TypedArray object
JSMSG_SHORT_TYPED_ARRAY_RETURNED: expected TypedArray of at least length {0}, but constructor returned TypedArray of length {1}