Closed Bug 1232966 Opened 4 years ago Closed 3 years ago

Implement %SharedArrayBuffer%.prototype.slice


(Core :: JavaScript Engine, defect, P2)




Tracking Status
firefox52 --- fixed


(Reporter: lth, Assigned: lth)


(Blocks 1 open bug)


(Keywords: dev-doc-complete)


(3 files, 1 obsolete file)

The slice method is required for standards conformance.
Attachment #8698958 - Flags: review?(arai.unmht)
Assignee: nobody → lhansen
Comment on attachment 8698958 [details] [diff] [review]

I'm going to do things in the opposite order (subclassing first, then slice).  Cancelling review for now.  Sorry for the trouble.
Attachment #8698958 - Flags: review?(arai.unmht)
See Also: → 1233040
Depends on: 1226762
The dev doc update once this lands is at least the following:
A new page about the slice method, see
Keywords: dev-doc-needed
Priority: -- → P2
Implements %SharedArrayBuffer%.prototype.slice, and also the species accessor which is needed for proper subclassing behavior.
Attachment #8698958 - Attachment is obsolete: true
Attachment #8805902 - Flags: review?(arai.unmht)
Test cases for %SharedArrayBuffer%.prototype.slice, and also one for byteLength.
Attachment #8805903 - Flags: review?(arai.unmht)
Comment on attachment 8805902 [details] [diff] [review]

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.
Attachment #8805902 - Flags: review?(arai.unmht) → review+
Comment on attachment 8805903 [details] [diff] [review]

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.
Attachment #8805903 - Flags: review?(arai.unmht) → review+
You will have to update the test for slice in test_xrayToJS.xul, that I just landed in bug 1130988.
sorry had to backout for test failures like
Flags: needinfo?(lhansen)
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.)
Flags: needinfo?(lhansen)
Backout by
Backed out changeset 6892ec7e77f3
Backed out changeset f16586fdc3fd for arm bustage
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.
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.
The fascinating failure is being addressed in bug 1314564.
Depends on: 1314564
Adaptation of Xrays test case.
Attachment #8807147 - Flags: review?(evilpies)
Comment on attachment 8807147 [details] [diff] [review]

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

I am glad to see this being fixed now!
Attachment #8807147 - Flags: review?(evilpies) → review+
Comment on attachment 8807147 [details] [diff] [review]

Boris, evilpie approved this but it probably needs your stamp - one more property going onto SharedArrayBuffer.
Flags: needinfo?(bzbarsky)
Comment on attachment 8807147 [details] [diff] [review]

Flags: needinfo?(bzbarsky)
Attachment #8807147 - Flags: review+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.