%TypedArray%.prototype.indexOf is coercing non-integers or non-floats to numbers wrongly

[Caio Lima]

Current implementation of %TypedArray%.prototype.indexOf is coercing the argument to Array type and generating strange results, which are potential bug generators. Check the following example:

>>> new UInt8Array([0, 1, 2]).indexOf("abc");
0

The return value is 0 because "abc" is being converted to 0. It makes more sense of the result is -1.

[Keith Miller]

Created attachment 283713 [details]
Patch

[Keith Miller]

Created attachment 283718 [details]
Patch

[Caio Lima]

(In reply to comment #1)
> Created attachment 283713 [details]
> Patch

Hey Keith, I took a look in your patch, but I can't comment there, so I am going to point the most important thing I saw =).
The verification:

if (!valueToFind.isNumber())
    return JSValue::encode(jsNumber(-1));

is not enough to implement indexOf because of 3 reasons:

1. It is against spec, because it doesn't perform toInteger. (https://tc39.github.io/ecma262/2016/#sec-array.prototype.indexof), so, if fromIndex is an object, valueOf need to be called. Take a look in this Benjamin's comment in my last patch for %TypedArray%.prototype.includes: https://bugs.webkit.org/show_bug.cgi?id=159385#c38 

2. Values that are numbers will be coerced improperly:
E.g. new Int32Array([1, 2, 3]).indexOf(2.5); // returns 1. Should return -1.

I created a new method in ToNativeFromValue.h with the following signature "bool toNativeFromValue(ExecState* exec, JSValue value, typename Adaptor::Type& result)". This method returns false if it is not possible convert the JSValue to nativeNumber in the Array. So, it already checks cases such as:

1. new UInt8Array([0]).indexOf(NaN); // should return -1. 
2. new  UInt8Array([0]).indexOf(256); // should return -1 because of UInt8 boundaries. 
2. new UInt8Array([2]).indexOf(2.5); // should return -1, because 2.5 is not an element of UInt8Array.
...

There are a lot of cases that need to be considered. The best source of this information is "LayoutTests/js/script-tests/typed-array-includes.js", where almost all ".includes" calls should be replaced to "indexOf" and when result should be false, it shall be -1.

3. NaN cases aren't being considered.

Hope helped =).

[Keith Miller]

(In reply to comment #3)
> (In reply to comment #1)
> > Created attachment 283713 [details]
> > Patch
> 
> Hey Keith, I took a look in your patch, but I can't comment there, so I am
> going to point the most important thing I saw =).
> The verification:
> 
> if (!valueToFind.isNumber())
>     return JSValue::encode(jsNumber(-1));
> 
> is not enough to implement indexOf because of 3 reasons:
> 
> 1. It is against spec, because it doesn't perform toInteger.
> (https://tc39.github.io/ecma262/2016/#sec-array.prototype.indexof), so, if
> fromIndex is an object, valueOf need to be called. Take a look in this
> Benjamin's comment in my last patch for %TypedArray%.prototype.includes:
> https://bugs.webkit.org/show_bug.cgi?id=159385#c38 
> 
> 2. Values that are numbers will be coerced improperly:
> E.g. new Int32Array([1, 2, 3]).indexOf(2.5); // returns 1. Should return -1.
> 
> I created a new method in ToNativeFromValue.h with the following signature
> "bool toNativeFromValue(ExecState* exec, JSValue value, typename
> Adaptor::Type& result)". This method returns false if it is not possible
> convert the JSValue to nativeNumber in the Array. So, it already checks
> cases such as:
> 
> 1. new UInt8Array([0]).indexOf(NaN); // should return -1. 
> 2. new  UInt8Array([0]).indexOf(256); // should return -1 because of UInt8
> boundaries. 
> 2. new UInt8Array([2]).indexOf(2.5); // should return -1, because 2.5 is not
> an element of UInt8Array.
> ...
> 
> There are a lot of cases that need to be considered. The best source of this
> information is "LayoutTests/js/script-tests/typed-array-includes.js", where
> almost all ".includes" calls should be replaced to "indexOf" and when result
> should be false, it shall be -1.
> 
> 3. NaN cases aren't being considered.
> 
> Hope helped =).

Good catch I forgot about those points. I'll fix.

[Keith Miller]

Created attachment 283781 [details]
Patch

[Geoffrey Garen]

Comment on attachment 283781 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 283781 [details]
Patch

Clearing flags on attachment: 283781

Committed r203297: <http://trac.webkit.org/changeset/203297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Keith Miller]

*** Bug 159241 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

<rdar://problem/27324561>