Bug 83818

Summary:

Incorrect TypedArray#set behavior

Product:

WebKit

Reporter:

Stéphan Kochen <stephan>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, fpizlo, ggaren, kbr, mark.lam, mhahnenberg, msaboff, oliver, sam, tonikitoo, zmo

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Test case	none
the patch	oliver: review+
test for the case where we optimize with memmove	none
test for overlapping elements of the same size	none
test where the source elements are smaller than the destination elements	none
test where the destination elements are smaller than the source elements	none
the patch	oliver: review+

Stéphan Kochen

Reported 2012-04-12 13:50:14 PDT

Created attachment 136964 [details] Test case Copying between two TypedArrays of different types, but backed by the same ArrayBuffer, using TypedArray#set can lead to incorrect results. The attached test case can be run straight in the Developer Tools console. It contains three asserts of which the third fails: Uint16 value: 1285 == 1285 ? true First Uint16 element: 5 == 5 ? true Second Uint16 element: 0 == 5 ? false I'm guessing a[0] is written before b[1] is read. According to the spec, the copy should behave as if a temporary buffer is inbetween. Tested on a nightly, that describes itself as: Version 5.1.5 (7534.55.3, r113983) Thread on WebGL mailing list: https://www.khronos.org/webgl/public-mailing-list/archives/1204/msg00172.html

Attachments
Test case (458 bytes, application/x-javascript) 2012-04-12 13:50 PDT, Stéphan Kochen	no flags	Details
the patch (7.20 KB, patch) 2013-08-22 22:58 PDT, Filip Pizlo	oliver: review+	Details Formatted Diff Diff
test for the case where we optimize with memmove (459 bytes, application/x-javascript) 2013-08-22 22:59 PDT, Filip Pizlo	no flags	Details
test for overlapping elements of the same size (482 bytes, application/x-javascript) 2013-08-22 23:00 PDT, Filip Pizlo	no flags	Details
test where the source elements are smaller than the destination elements (2.65 KB, application/x-javascript) 2013-08-22 23:00 PDT, Filip Pizlo	no flags	Details
test where the destination elements are smaller than the source elements (1.56 KB, application/x-javascript) 2013-08-22 23:01 PDT, Filip Pizlo	no flags	Details
the patch (28.14 KB, patch) 2013-08-23 13:17 PDT, Filip Pizlo	oliver: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Oliver Hunt

Comment 1 2012-04-12 14:10:55 PDT

Yay for a spec that claims performance is important, but actively sabotages optimisations.

Kenneth Russell

Comment 2 2012-04-17 17:52:51 PDT

As discussed on the public_webgl list, the original intent of the typed array spec was that this would throw TypeError. set() taking a typed array was supposed to be implementable using memmove(). Unfortunately, it turns out that this overload was illegal according to Web IDL. Recent Web IDL spec changes make the overload legal, but require that the version taking a JS array be called for all typed array view types other than the receiver's.

Filip Pizlo

Comment 3 2013-08-22 19:17:51 PDT

I'll look at it.

Filip Pizlo

Comment 4 2013-08-22 22:58:28 PDT

Created attachment 209432 [details] the patch Note, I have a bunch of tests for this - I'll upload the ad-hoc tests I wrote and I will LayoutTest-ify them later. I plan to also LayoutTest-ify Stéphan's original test case.

Filip Pizlo

Comment 5 2013-08-22 22:59:39 PDT

Created attachment 209433 [details] test for the case where we optimize with memmove

Filip Pizlo

Comment 6 2013-08-22 23:00:07 PDT

Created attachment 209434 [details] test for overlapping elements of the same size

Filip Pizlo

Comment 7 2013-08-22 23:00:36 PDT

Created attachment 209435 [details] test where the source elements are smaller than the destination elements

Filip Pizlo

Comment 8 2013-08-22 23:01:01 PDT

Created attachment 209436 [details] test where the destination elements are smaller than the source elements

Filip Pizlo

Comment 9 2013-08-23 13:17:57 PDT

Created attachment 209501 [details] the patch The patch including tests and some minor changes.

Mark Hahnenberg

Comment 10 2013-08-23 13:35:26 PDT

Comment on attachment 209501 [details] the patch r=me too

Filip Pizlo

Comment 11 2013-08-23 13:41:21 PDT

Landed in http://trac.webkit.org/changeset/154518

Note You need to log in before you can comment on or make changes to this bug.