Bug 184232 – Typed array constructor behaves differently when length is not passed or when undefined is passed

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Koby]

Hi
I've noticed that when I create a typed array, the resulting array's byteLength is different if I'm not passing a length, or if I'm passing an undefined length:
const buffer = new ArrayBuffer(8);
const view1 = new Uint8Array(buffer); // byteLength == 8
const view2 = new Uint8Array(buffer, undefined, undefined); // byteLength == 0

Tested with the JSC shell from my WebKit fork, forked on github on February, and with Safari on iOS 11.2.6.

According to to the ecma standard (https://www.ecma-international.org/ecma-262/6.0/#sec-%typedarray%-buffer-byteoffset-length) I think they should produce typed arrays with the same byteLength.
I've verified it with Firefox nightly and Chrome nightly (both on Windows), both producing a Uint8Array with the same byteLength(8).

Going through JSC's source, I think the issue is in constructGenericTypedArrayView in runtime/JSGenericTypedArrayViewConstructorInlines.h. It handles undefined byteLength for DataViews, but not the "length" for other typed arrays.

Thanks,
Koby

[Radar WebKit Bug Importer]

<rdar://problem/39145280>

[Koby]

Also, if you think that my suggested fix in constructGenericTypedArrayView is OK, I'll be happy to send a patch.

[James Darpinian]

Thanks for the investigation, I think you are right about the fix. This is causing some WebGL conformance tests to fail so I will make a patch.

[James Darpinian]

Created attachment 401806 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 401806 [details]
Patch

Nice, can you add a test in `JSTests/stress/`?

[Yusuke Suzuki]

And let's wait for EWS results. If it affects on WebGL conformance tests, it is possible that these tests in LayoutTests start showing "PASS" or something. In that case, this patch needs to update -expect files too.

[Yusuke Suzuki]

Seems that it is not used in LayoutTests for now. So, adding a test to JSTests/stress/ and that's all :)

[Yusuke Suzuki]

Comment on attachment 401806 [details]
Patch

The patch looks good. Can you add a test to JSTests/stress?

[James Darpinian]

Created attachment 402364 [details]
add test

[Yusuke Suzuki]

Comment on attachment 402364 [details]
add test

r=me

[EWS]

Committed r263315: <https://trac.webkit.org/changeset/263315>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402364 [details].