Re: [heycam/webidl] Add and improve operations on BufferSources (#987) from Timothy Gu on 2021-06-23 (public-webapps-github@w3.org from June 2021)

From: Timothy Gu <notifications@github.com>
Date: Wed, 23 Jun 2021 14:11:21 -0700
To: heycam/webidl <webidl@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <heycam/webidl/pull/987/review/691116582@github.com>
@TimothyGu commented on this pull request.

Suggestions for a few more concepts to define:

1. BufferSource/is detached: basically an IDL version of IsDetachedBuffer that also supports views.
2. ArrayBufferView/get the underlying ArrayBuffer: spares specs from having to step into ES land in order to transfer the input buffer.
3. (possibly) ArrayBufferView/transfer

> -    1.  If <a abstract-op>IsDetachedBuffer</a>(|arrayBuffer|) is <emu-val>true</emu-val>, then
-        return the empty byte sequence.
-    1.  Let |data| be the value of |O|’s \[[ArrayBufferData]] [=internal slot=].
-    1.  Return a reference to or copy of (as required) the |length| bytes in |data|
-        starting at byte offset |offset|.
+<hr>
+
+<div algorithm>
+    To <dfn export for="ArrayBuffer">create</dfn> an {{ArrayBuffer}} from a [=byte sequence=]
+    |bytes| in a [=Realm=] |realm|:
+
+    1.  Let |arrayBuffer| be [=?=]
+        [$AllocateArrayBuffer$](|realm|.\[[Intrinsics]].[[{{%ArrayBuffer%}}]], |bytes|'s
+        [=byte sequence/length=]).
+    1. [=ArrayBuffer/Write=] |bytes| into |arrayBuffer|.
+    1. Return |arrayBuffer|.

It's unclear if these algorithms are operating on IDL values or ES values. Assuming IDL, then we need to first say

> Let _idlArrayBuffer_ be the `ArrayBuffer` value that is a reference to the same object as _arrayBuffer_.

or

> Let _idlArrayBuffer_ be the result of [converting](https://heycam.github.io/webidl/#dfn-convert-ecmascript-to-idl-value) _arrayBuffer_ to `ArrayBuffer`.

and subsequently write _bytes_ into _idlArrayBuffer_.

> -    1.  Otherwise, set |length| to the value of |O|’s \[[ArrayBufferByteLength]] [=internal slot=].
-    1.  If <a abstract-op>IsDetachedBuffer</a>(|arrayBuffer|) is <emu-val>true</emu-val>, then
-        return the empty byte sequence.
-    1.  Let |data| be the value of |O|’s \[[ArrayBufferData]] [=internal slot=].
-    1.  Return a reference to or copy of (as required) the |length| bytes in |data|
-        starting at byte offset |offset|.
+<hr>
+
+<div algorithm>
+    To <dfn export for="ArrayBuffer">create</dfn> an {{ArrayBuffer}} from a [=byte sequence=]
+    |bytes| in a [=Realm=] |realm|:
+
+    1.  Let |arrayBuffer| be [=?=]
+        [$AllocateArrayBuffer$](|realm|.\[[Intrinsics]].[[{{%ArrayBuffer%}}]], |bytes|'s
+        [=byte sequence/length=]).
+    1. [=ArrayBuffer/Write=] |bytes| into |arrayBuffer|.

```suggestion
    1.  [=ArrayBuffer/Write=] |bytes| into |arrayBuffer|.
```

Same for next line.

>  
-    To <dfn id="dfn-detach" for="ArrayBuffer" export>detach</dfn> an {{ArrayBuffer}}, these steps must be followed:
+    1.  Assert: if the type is not {{DataView}}, then |bytes|'s [=byte sequence/length=] [=modulo=]
+        the [=element size=] of that type is 0.
+    1.  Let |arrayBuffer| be the result of [=ArrayBuffer/creating=] an {{ArrayBuffer}} from |bytes|
+        in |realm|.
+    1.  Let |constructor| be the appropriate constructor from |realm|.\[[Intrinsics]] for the type
+        of {{ArrayBufferView}} being created.
+    1.  Return [=!=] [$Construct$](|constructor|, « |arrayBuffer| »).

Ditto here, this needs a ES-to-IDL conversion.

> +    given a {{BufferSource}} |bufferSource|:
+
+    1.  Let |arrayBuffer| be |bufferSource|.
+    1.  Let |offset| be 0.
+    1.  Let |length| be 0.
+    1.  If |bufferSource| has a \[[ViewedArrayBuffer]] [=internal slot=], then:
+        1.  Set |arrayBuffer| to |bufferSource|.\[[ViewedArrayBuffer]].
+        1.  Set |offset| to |bufferSource|.\[[ByteOffset]].
+        1.  Set |length| to |bufferSource|.\[[ByteLength]].
+    1.  Otherwise, set |length| to |bufferSource|.\[[ArrayBufferByteLength]].
+    1.  If [=!=] [$IsDetachedBuffer$](|arrayBuffer|) is true, then return the empty
+        [=byte sequence=].
+    1.  Let |bytes| be a new [=byte sequence=] of [=byte sequence/length=] equal to |length|.
+    1.  For |i| in [=the range=] |offset| to |offset| + |length| &minus; 1, inclusive, set
+        |bytes|[|i| &minus; |offset|] to [=!=] [$GetValueFromBuffer$](|arrayBuffer|, |i|, Uint8,
+        true, Unordered).

Is this missing a "Return _bytes_."?

>  
-    To <dfn id="dfn-detach" for="ArrayBuffer" export>detach</dfn> an {{ArrayBuffer}}, these steps must be followed:
+    1.  Assert: if the type is not {{DataView}}, then |bytes|'s [=byte sequence/length=] [=modulo=]
+        the [=element size=] of that type is 0.
+    1.  Let |arrayBuffer| be the result of [=ArrayBuffer/creating=] an {{ArrayBuffer}} from |bytes|
+        in |realm|.
+    1.  Let |constructor| be the appropriate constructor from |realm|.\[[Intrinsics]] for the type
+        of {{ArrayBufferView}} being created.
+    1.  Return [=!=] [$Construct$](|constructor|, « |arrayBuffer| »).
+</div>
+
+<div algorithm>
+    To <dfn id="dfn-get-buffer-source-copy" export lt="get a copy of the buffer source">get a copy of the bytes held by the buffer source</dfn>
+    given a {{BufferSource}} |bufferSource|:
+
+    1.  Let |arrayBuffer| be |bufferSource|.

Convert _bufferSource_ to a ES value first.

> +        of {{ArrayBufferView}} being created.
+    1.  Return [=!=] [$Construct$](|constructor|, « |arrayBuffer| »).
+</div>
+
+<div algorithm>
+    To <dfn id="dfn-get-buffer-source-copy" export lt="get a copy of the buffer source">get a copy of the bytes held by the buffer source</dfn>
+    given a {{BufferSource}} |bufferSource|:
+
+    1.  Let |arrayBuffer| be |bufferSource|.
+    1.  Let |offset| be 0.
+    1.  Let |length| be 0.
+    1.  If |bufferSource| has a \[[ViewedArrayBuffer]] [=internal slot=], then:
+        1.  Set |arrayBuffer| to |bufferSource|.\[[ViewedArrayBuffer]].
+        1.  Set |offset| to |bufferSource|.\[[ByteOffset]].
+        1.  Set |length| to |bufferSource|.\[[ByteLength]].
+    1.  Otherwise, set |length| to |bufferSource|.\[[ArrayBufferByteLength]].

```suggestion
    1.  Otherwise:
        1. Assert: |bufferSource| is an {{ECMAScript/ArrayBuffer}} object.
        1. Set |length| to |bufferSource|.\[[ArrayBufferByteLength]].
```

> @@ -14285,7 +14343,7 @@ A {{DOMException}} is represented by a
 
 <div algorithm="to create a simple exception or DOMException">
 
-    To [=create=] a [=simple exception=] or {{DOMException}} |E|, with a string giving the
+    To create a [=simple exception=] or {{DOMException}} |E|, with a string giving the

Unrelated? FWIW, this link is a good one.

> +<p class="advisement">Extreme care must be taken when writing specification text that
+[=ArrayBuffer/writes=] into an {{ArrayBuffer}} or {{ArrayBufferView}}, as the underlying data can
+easily be changed by the script author or other APIs at unpredictable times. This is especially true
+if the [{{AllowShared}}] [=extended attribute=] is involved. For the non-shared cases, it is
+recommended to [=ArrayBuffer/transfer=] the {{ArrayBuffer}} first if possible, to ensure the writes
+cannot overlap with other modifications, and then give the new {{ArrayBuffer}} instance to author
+code as necessary.
+
+<div algorithm>
+    To <dfn id="dfn-detach" for="ArrayBuffer" export>detach</dfn> an {{ArrayBuffer}} |arrayBuffer|:
+
+    1.  Assert: [=!=] [$IsSharedArrayBuffer$](|arrayBuffer|) is false.
+    1.  Perform [=?=] [$DetachArrayBuffer$](|arrayBuffer|).
+
+    <p class="note">This will throw an exception if |arrayBuffer| has an \[[ArrayBufferDetachKey]]
+    that is not undefined, such as a {{Memory|WebAssembly.Memory}}'s {{Memory/buffer}}.

```suggestion
    that is not undefined, such as a {{Memory|WebAssembly.Memory}}'s {{Memory/buffer}} attribute.
```

> +code as necessary.
+
+<div algorithm>
+    To <dfn id="dfn-detach" for="ArrayBuffer" export>detach</dfn> an {{ArrayBuffer}} |arrayBuffer|:
+
+    1.  Assert: [=!=] [$IsSharedArrayBuffer$](|arrayBuffer|) is false.
+    1.  Perform [=?=] [$DetachArrayBuffer$](|arrayBuffer|).
+
+    <p class="note">This will throw an exception if |arrayBuffer| has an \[[ArrayBufferDetachKey]]
+    that is not undefined, such as a {{Memory|WebAssembly.Memory}}'s {{Memory/buffer}}.
+    [[WASM-JS-API-1]]
+</div>
+
+<div algorithm>
+    To <dfn for="ArrayBuffer" export>transfer</dfn> an {{ArrayBuffer}} |arrayBuffer|, optionally
+    given a [=Realm=] |realm|:

```suggestion
    given a [=Realm=] |targetRealm|:
```

Unlike creation algorithms, it's not immediately clear what the realm is used for.

Also fix usages below.

> +    <dfn export for="ArrayBufferView/write">|startingOffset|</dfn> (default 0):
+
+    1.  Assert: |bytes|'s [=byte sequence/length=] ≤ |view|.\[[ByteLength]] &minus;
+        |startingOffset|.
+    1.  Assert: if |view| is not a {{DataView}}, then |bytes|'s [=byte sequence/length=] [=modulo=]
+        the [=element size=] of |view|'s type is 0.
+    1.  [=ArrayBuffer/Write=] |bytes| into |view|.\[[ViewedArrayBuffer]] with
+        <i>[=ArrayBuffer/write/startingOffset=]</i> set to |view|.\[[ByteOffset]] +
+        |startingOffset|.
+</div>
+
+<p class="advisement">Extreme care must be taken when writing specification text that
+[=ArrayBuffer/writes=] into an {{ArrayBuffer}} or {{ArrayBufferView}}, as the underlying data can
+easily be changed by the script author or other APIs at unpredictable times. This is especially true
+if the [{{AllowShared}}] [=extended attribute=] is involved. For the non-shared cases, it is
+recommended to [=ArrayBuffer/transfer=] the {{ArrayBuffer}} first if possible, to ensure the writes

In addition to transferring, we can also recommend copying here, right?

> +    1.  Let |transferred| be [=!=]
+        [$OrdinaryCreateFromConstructor$](|realm|.\[[Intrinsics]].[[{{%ArrayBuffer%}}]],
+        "%ArrayBuffer.prototype%", « \[[ArrayBufferData]], \[[ArrayBufferByteLength]],
+        \[[ArrayBufferDetachKey]] »).

Having a list of internal slots here seems relatively brittle. Can we say

> Let _transferred_ be ? [AllocateArrayBuffer](https://tc39.es/ecma262/#sec-allocatearraybuffer)(_realm_.[[Intrinsics]].[[%ArrayBuffer%]], 0).

> +
+<p class="advisement">Extreme care must be taken when writing specification text that
+[=ArrayBuffer/writes=] into an {{ArrayBuffer}} or {{ArrayBufferView}}, as the underlying data can
+easily be changed by the script author or other APIs at unpredictable times. This is especially true
+if the [{{AllowShared}}] [=extended attribute=] is involved. For the non-shared cases, it is
+recommended to [=ArrayBuffer/transfer=] the {{ArrayBuffer}} first if possible, to ensure the writes
+cannot overlap with other modifications, and then give the new {{ArrayBuffer}} instance to author
+code as necessary.
+
+<div algorithm>
+    To <dfn id="dfn-detach" for="ArrayBuffer" export>detach</dfn> an {{ArrayBuffer}} |arrayBuffer|:
+
+    1.  Assert: [=!=] [$IsSharedArrayBuffer$](|arrayBuffer|) is false.
+    1.  Perform [=?=] [$DetachArrayBuffer$](|arrayBuffer|).
+
+    <p class="note">This will throw an exception if |arrayBuffer| has an \[[ArrayBufferDetachKey]]

Might be good to note that detaching an ArrayBuffer that's already detached is a no-op.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/heycam/webidl/pull/987#pullrequestreview-691116582
Received on Wednesday, 23 June 2021 21:11:42 UTC