Closed Bug 1314564 Opened 3 years ago Closed 3 years ago

Implement SharedArrayBuffer meta-properties

Categories

(Core :: JavaScript: Standard Library, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(3 files, 1 obsolete file)

We need the toStringTag and the species.

We probably want these uplifted to FF51, but it is probably not critical.
WIP.

This patch exposes the test case failure that is being investigated as part of bug 1232966; it is certainly something to do with how subclassing works.  I suspect this problem has something to do with GetSpeciesConstructor in TypedArrayObject.cpp, because that does not look straightforward at all and has a case only for ArrayBuffer, not for SharedArrayBuffer.
The reason for the test case failure is that a TypedArray is constructed with an ArrayBuffer payload, but where the ArrayBuffer has the SharedArrayBufferPrototype as the prototype object.

The reason for that is that when the TypedArray construction machinery goes to work it calls on GetSpeciesConstructor to get a constructor from which to extract the prototype object; GetSpeciesConstructor in turn calls SpeciesConstructor on the object if the object is not an ArrayBuffer already.  This is wrong: since an ArrayBuffer is to be constructed always, GetSpeciesConstructor must return the ArrayBuffer constructor if the input payload is a SharedArrayBuffer.
This patch makes the test failure go away, but I'm uncertain about whether it's actually the appropriate fix.

The important changes are in GetSpeciesConstructor, where I introduce guards on IsSharedArrayBufferSpecies in two places.  The first of these feels right, since there's already a guard there for IsArrayBufferSpecies, but the second is less obvious.  For one thing, one could guard on the return value from SpeciesConstructor instead.  But in general, I'm not sure what's exactly right here.
Attachment #8807142 - Flags: review?(arai.unmht)
Attachment #8807142 - Flags: feedback?(jwalden+bmo)
Attachment #8807142 - Flags: feedback?(andrebargull)
Attachment #8806656 - Flags: review?(arai.unmht)
Blocks: 1232966
Try run as part of this larger patch queue:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=baef773552e1
(In reply to Lars T Hansen [:lth] from comment #1)
> I suspect this problem has something to do with GetSpeciesConstructor in
> TypedArrayObject.cpp, because that does not look straightforward at all and
> has a case only for ArrayBuffer, not for SharedArrayBuffer.

IsArrayBufferSpecies check is there just for optimization.
It shouldn't have any effect on the semantics.
(In reply to Tooru Fujisawa [:arai] from comment #5)
> (In reply to Lars T Hansen [:lth] from comment #1)
> > I suspect this problem has something to do with GetSpeciesConstructor in
> > TypedArrayObject.cpp, because that does not look straightforward at all and
> > has a case only for ArrayBuffer, not for SharedArrayBuffer.
> 
> IsArrayBufferSpecies check is there just for optimization.
> It shouldn't have any effect on the semantics.

I was afraid of that.  So the change likely needs to be somewhere else, but picking up the wrong prototype as we do now is definitely not desirable.
This is actually an issue in the test file (or in the Shared Memory spec). The outer loop in "tests/ecma_6/TypedArray/Tconstructor-fromTypedArray-byteLength.js" should iterate over "typedArrayConstructors" instead of "anyTypedArrayConstructors".

Or the Shared Memory spec needs to update 22.2.4.3, steps 17-18 [1] to handle typed arrays with SharedArrayBuffers. The test basically executes `new Int8Array(new Int8Array(new SharedArrayBuffer(0))).buffer.byteLength`, and per the current spec proposal, this expression throws a TypeError exception. 

[1] https://tc39.github.io/ecma262/#sec-typedarray-typedarray
(In reply to André Bargull from comment #7)
> This is actually an issue in the test file (or in the Shared Memory spec).
> The outer loop in
> "tests/ecma_6/TypedArray/Tconstructor-fromTypedArray-byteLength.js" should
> iterate over "typedArrayConstructors" instead of "anyTypedArrayConstructors".
> 
> Or the Shared Memory spec needs to update 22.2.4.3, steps 17-18 [1] to
> handle typed arrays with SharedArrayBuffers. The test basically executes
> `new Int8Array(new Int8Array(new SharedArrayBuffer(0))).buffer.byteLength`,
> and per the current spec proposal, this expression throws a TypeError
> exception. 
> 
> [1] https://tc39.github.io/ecma262/#sec-typedarray-typedarray

Are you saying that it is a reasonable outcome for the ArrayBuffer to be constructed with the SharedArrayBufferPrototype as I described above?  It's surprising to me, but ...

The intent of the Shared Memory spec is that the TypedArray methods should construct plain ArrayBuffers in this case.  I'll take a look at the prose you point to.
(In reply to Lars T Hansen [:lth] from comment #8)
> The intent of the Shared Memory spec is that the TypedArray methods should
> construct plain ArrayBuffers in this case.  I'll take a look at the prose
> you point to.

In that case, SharedArrayBuffer[@@species] should return ArrayBuffer, I think.
so the change to GetSpeciesConstructor in the patch makes sense.
(In reply to Tooru Fujisawa [:arai] from comment #9)
> (In reply to Lars T Hansen [:lth] from comment #8)
> > The intent of the Shared Memory spec is that the TypedArray methods should
> > construct plain ArrayBuffers in this case.  I'll take a look at the prose
> > you point to.
> 
> In that case, SharedArrayBuffer[@@species] should return ArrayBuffer, I
> think.
> so the change to GetSpeciesConstructor in the patch makes sense.

I suppose that might fix this problem, but I'm afraid that will set us up for other problems later.  The issue here should not be with SharedArrayBuffer, but with TypedArray -- by committee agreement, TypedArray should not construct a SharedArrayBuffer, but TypedArray must handle cases where the input buffer is a SharedArrayBuffer as if it were an ArrayBuffer (and produce a regular ArrayBuffer).

But that should have no bearing on other uses of SharedArrayBuffer IMO; if other code finds reason to use SharedArrayBuffer[@@species] they should find that SharedArrayBuffer looks "normal".
(In reply to Lars T Hansen [:lth] from comment #8)
> The intent of the Shared Memory spec is that the TypedArray methods should
> construct plain ArrayBuffers in this case.  I'll take a look at the prose
> you point to.

Okay. In that case, the Shared Memory spec needs to specify the following changes to 22.2.4.3, steps 17-18:
---
17. If SameValue(elementType, srcType) is true, then
    a. Let srcLength be typedArray.[[ByteLength]].
    b. If IsSharedArrayBuffer(srcData) is false, then
        i. Let data be ? CloneArrayBuffer(srcData, srcByteOffset, srcLength).
    c. Else,
        i. Let data be ? CloneArrayBuffer(srcData, srcByteOffset, srcLength, %ArrayBuffer%).
18. Else,
    a. If IsSharedArrayBuffer(srcData) is false, then
        i. Let bufferConstructor be ? SpeciesConstructor(srcData, %ArrayBuffer%).
    b. Else,
        i. Let bufferConstructor be %ArrayBuffer%.
    b. Let data be ? AllocateArrayBuffer(bufferConstructor, byteLength).
---
(In reply to André Bargull from comment #11)
> (In reply to Lars T Hansen [:lth] from comment #8)
> > The intent of the Shared Memory spec is that the TypedArray methods should
> > construct plain ArrayBuffers in this case.  I'll take a look at the prose
> > you point to.
> 
> Okay. In that case, the Shared Memory spec needs to specify the following changes to 22.2.4.3, steps 17-18:

...

Thank you.  That looks plausible.  Filed as https://github.com/tc39/ecmascript_sharedmem/issues/149.
We also need to correct an Xrays test here because of the introduction of toStringTag, see https://treeherder.mozilla.org/#/jobs?repo=try&revision=baef773552e1&selectedJob=30391222&filter-searchStr=Linux%20x64%20opt%20Mochitests%20executed%20by%20TaskCluster%20desktop-test-linux64%2Fopt-mochitest-chrome-3%20tc-M(c3)

TEST-UNEXPECTED-FAIL | js/xpconnect/tests/chrome/test_xrayToJS.xul | A symbol-keyed property on the SharedArrayBuffer prototype has been changed! You need a security audit from an XPConnect peer - got "[\"Symbol.toStringTag\"]", expected "[]"
TEST-UNEXPECTED-FAIL | js/xpconnect/tests/chrome/test_xrayToJS.xul | getOwnPropertySymbols works - got "[\"Symbol.toStringTag\"]", expected "[]"
Corrected as suggested.
Attachment #8807142 - Attachment is obsolete: true
Attachment #8807142 - Flags: review?(arai.unmht)
Attachment #8807142 - Flags: feedback?(jwalden+bmo)
Attachment #8807142 - Flags: feedback?(andrebargull)
Attachment #8807461 - Flags: review?(arai.unmht)
Attachment #8807461 - Flags: review?(andrebargull)
Adapt xray test to include new properties.
Attachment #8807463 - Flags: review?(evilpies)
Comment on attachment 8807461 [details] [diff] [review]
bug1314564-override-constructor.patch

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

looks good, except the change to JS_WrapValue.

::: js/src/vm/TypedArrayObject.cpp
@@ +1093,3 @@
>              return false;
> +
> +        return JS_WrapValue(cx, ctor);

this should be outside of JSAutoCompartment scope,
otherwise it does nothing.

Please revert this line.
Attachment #8807461 - Flags: review?(arai.unmht) → review+
Attachment #8806656 - Flags: review?(arai.unmht) → review+
Comment on attachment 8807463 [details] [diff] [review]
bug1314564-xray-test.patch

LGTM, but we should probably not violate that big fat comment in this file :)
Attachment #8807463 - Flags: review?(evilpies) → feedback+
Comment on attachment 8807461 [details] [diff] [review]
bug1314564-override-constructor.patch

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

r=me with the JS_WrapValue bit fixed.
Attachment #8807461 - Flags: review?(andrebargull) → review+
Comment on attachment 8807463 [details] [diff] [review]
bug1314564-xray-test.patch

Boris, perhaps you can spare a minute to approve this: 

New properties on SharedArrayBuffer must be recognized by Xray tests.
Flags: needinfo?(bzbarsky)
Comment on attachment 8807463 [details] [diff] [review]
bug1314564-xray-test.patch

r=me
Flags: needinfo?(bzbarsky)
Attachment #8807463 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b39f69581735e25f53b69ec51263f80a1a682a
Bug 1314564 - toStringTag and species for SharedArrayBuffer. r=arai

https://hg.mozilla.org/integration/mozilla-inbound/rev/7185f2eb656a242c0bf4b56ac2f9731dc833bbdb
Bug 1314564 - adapt xray tests for SharedArrayBuffer to include new properties. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e3ae9ca5f809c355799fe04e4f81315a8104f3d
Bug 1314564 - override constructor for TypedArray construction when argument is shared memory. r=arai
https://hg.mozilla.org/mozilla-central/rev/b2b39f695817
https://hg.mozilla.org/mozilla-central/rev/7185f2eb656a
https://hg.mozilla.org/mozilla-central/rev/3e3ae9ca5f80
Status: ASSIGNED → RESOLVED
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.