1232966 - Implement %SharedArrayBuffer%.prototype.slice

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Lars T Hansen [:lth]]

The slice method is required for standards conformance.

Comment 1

[Lars T Hansen [:lth]]

Attachment: SharedArrayBuffer.prototype.slice

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8698958 [details] [diff] [review]
SharedArrayBuffer.prototype.slice

I'm going to do things in the opposite order (subclassing first, then slice).  Cancelling review for now.  Sorry for the trouble.

Comment 3

[Florian Scholz (Open Web Docs)]

The dev doc update once this lands is at least the following:
A new page about the slice method, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer#SharedArrayBuffer_prototype_object

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1232966-sab-slice.patch

Implements %SharedArrayBuffer%.prototype.slice, and also the species accessor which is needed for proper subclassing behavior.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1232966-sab-slice-tests.patch

Test cases for %SharedArrayBuffer%.prototype.slice, and also one for byteLength.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8805902 [details] [diff] [review]
bug1232966-sab-slice.patch

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

::: js/src/vm/CommonPropertyNames.h
@@ +273,5 @@
>      macro(SetIterator, SetIterator, "Set Iterator") \
>      macro(setPrefix, setPrefix, "set ") \
>      macro(setPrototypeOf, setPrototypeOf, "setPrototypeOf") \
>      macro(shape, shape, "shape") \
> +    macro(SharedArrayBufferSpecies, SharedArrayBufferSpecies, "SharedArrayBufferSpecies") \

This isn't used anywhere in this patch.
would be better removing for now.

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8805903 [details] [diff] [review]
bug1232966-sab-slice-tests.patch

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

::: js/src/jit-test/tests/sharedbuf/methods.js
@@ +4,5 @@
> +    quit(0);
> +
> +load(libdir + "asserts.js");
> +
> +// Test the byteLength method

It should be better separating files per each method.
byteLength.js and slice.js, or something like that with prefix.

so that you don't need the block statement, and it would become clear what goes wrong if some of them hit regression.

@@ +7,5 @@
> +
> +// Test the byteLength method
> +
> +{
> +    let v = new SharedArrayBuffer(137);

can this variable be named `buffer` or something?
same for other 1-2 length variables (except `i`), they need more descriptive name.

Comment 8

[Tom S [:evilpie]]

You will have to update the test for slice in test_xrayToJS.xul, that I just landed in bug 1130988.

Comment 9

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f16586fdc3fd11dbf541f1f8b12296165b3b0d86
Bug 1232966 - SharedArrayBuffer.prototype.slice. r=arai

https://hg.mozilla.org/integration/mozilla-inbound/rev/6892ec7e77f3e244473498e3c0c3731939227170
Bug 1232966 - test cases for SharedArrayBuffer.prototype.slice. r=arai

Comment 10

[Carsten Book [:Tomcat]]

sorry had to backout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38469517&repo=mozilla-inbound

Comment 11

[Lars T Hansen [:lth]]

In a debug build, fails with --ion-eager --ion-offthread-compile=off:

Assertion failure: fromBuffer->byteLength() >= fromIndex + count, at /Users/lhansen/moz/mozilla-inbound/js/src/vm/SharedArrayObject.cpp:337

(Will also add a patch for Tom's case.)

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3334bb262d3
Backed out changeset 6892ec7e77f3 
https://hg.mozilla.org/integration/mozilla-inbound/rev/017b9ab11023
Backed out changeset f16586fdc3fd for arm bustage

Comment 13

[Lars T Hansen [:lth]]

A failure on my part to set up inlinable intrinsics correctly.  There's really no particular reason for these to be inlinable; they are not going to be hot.  So it's an easy fix.

Comment 14

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=025f7c04d4db

Comment 15

[Lars T Hansen [:lth]]

With this patch (suitably corrected) there's a fascinating new failure in an existing test: in ecma_6/TypedArray/Tconstructor-fromTypedArray-byteLength.js.

The direct cause of the failure is that an object that is an ArrayBuffer (it prints as "[ArrayBuffer]" and believes itself to be that) has a prototype that is SharedArrayBuffer.prototype.  Hence when we call the byteLength getter on that object there's a failure, since the byteLength getter is nongeneric and SharedArrayBuffer's byteLength fails the type check when presented with an arrayBuffer.

How we end up with the wrong prototype is unknown.  What is known is that the patch on the present bug fixes subclassing of SharedArrayBuffer, which was not previously working.  There could be a bug in that fix, or the fix could be uncovering a previous bug, either in the VM or in the test case.  The test case relies on some particularly gnarly reflection to construct a set of constructors for TypedArrays that carry SharedArrayBuffers...  See ecma_{6,7}/TypedArray/shell.js.

Comment 16

[Lars T Hansen [:lth]]

The fascinating failure is being addressed in bug 1314564.

Comment 17

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3f809bf3a28

Comment 18

[Lars T Hansen [:lth]]

Attachment: bug1232966-sab-slice-xray.patch

Adaptation of Xrays test case.

Comment 19

[Tom S [:evilpie]]

Comment on attachment 8807147 [details] [diff] [review]
bug1232966-sab-slice-xray.patch

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

I am glad to see this being fixed now!

Comment 20

[Lars T Hansen [:lth]]

Comment on attachment 8807147 [details] [diff] [review]
bug1232966-sab-slice-xray.patch

Boris, evilpie approved this but it probably needs your stamp - one more property going onto SharedArrayBuffer.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8807147 [details] [diff] [review]
bug1232966-sab-slice-xray.patch

r=me

Comment 22

[Lars T Hansen [:lth]]

Try build is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95505b97467d

Comment 23

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/334620bedacab85bcfed7934a714d95d9945455d
Bug 1232966 - SharedArrayBuffer.prototype.slice. r=arai

https://hg.mozilla.org/integration/mozilla-inbound/rev/55398c6f895777f83bdec5cc4bbef3af9f958930
Bug 1232966 - test cases for SharedArrayBuffer.prototype.slice. r=arai

https://hg.mozilla.org/integration/mozilla-inbound/rev/00c0fdd359040cf9c5515522a620f06ea26cc6ba
Bug 1232966 - Adapt Xrays tests for SharedArrayBuffer.prototype.slice. r=bz

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/334620bedaca
https://hg.mozilla.org/mozilla-central/rev/55398c6f8957
https://hg.mozilla.org/mozilla-central/rev/00c0fdd35904

Comment 25

[Florian Scholz (Open Web Docs)]

New page: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer/slice

Updated: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer#Methods

As this isn't in a Firefox release yet, it won't appear on the "Firefox for developers" release notes. Shared memory is listed as an experimental feature, though: https://developer.mozilla.org/en-US/Firefox/Experimental_features#JavaScript