Re: [whatwg/streams] Various fixes for readable byte streams (#1123)

@MattiasBuelens commented on this pull request.

Okay, I think it *will* work to remove `CanTransferArrayBuffer`! 😁

> @@ -48,6 +49,9 @@ exports.implementation = class ReadableByteStreamControllerImpl {
     if (chunk.buffer.byteLength === 0) {
       throw new TypeError('chunk\'s buffer must have non-zero byteLength');
     }
+    if (CanTransferArrayBuffer(chunk.buffer) === false) {

This check can easily be removed. We can move [these lines](https://github.com/whatwg/streams/blob/5da8d5c2bfdde4e3ce34ec3e63f4bd3f86922390/reference-implementation/lib/abstract-ops/readable-streams.js#L1010-L1013) in `ReadableByteStreamControllerEnqueue` to the start of that function, so that `TransferArrayBuffer` happens earlier (before we invalidate the BYOB request).

> @@ -14,7 +14,7 @@ exports.implementation = class ReadableStreamBYOBRequestImpl {
       throw new TypeError('This BYOB request has been invalidated');
     }
 
-    if (IsDetachedBuffer(this._view.buffer) === true) {
+    if (CanTransferArrayBuffer(this._view.buffer) === false) {

We can turn this back into a regular `IsDetachedBuffer` check without problems. `this._view` is always an `Uint8Array` that we constructed ourselves around the buffer of the first pull-into descriptor, which we have been checked earlier (i.e. received from `BYOBReader.read(view)` or created ourselves through `autoAllocateChunkSize`).

> @@ -17,6 +18,9 @@ class ReadableStreamBYOBReaderImpl {
     if (view.buffer.byteLength === 0) {
       return promiseRejectedWith(new TypeError('view\'s buffer must have non-zero byteLength'));
     }
+    if (CanTransferArrayBuffer(view.buffer) === false) {

This one can also be changed to `IsDetachedBuffer`. We need to change `ReadableByteStreamControllerPullInto` to do a `try..catch` around [this line](https://github.com/whatwg/streams/blob/5da8d5c2bfdde4e3ce34ec3e63f4bd3f86922390/reference-implementation/lib/abstract-ops/readable-streams.js#L1175), and then immediately call `readIntoRequest.errorSteps()`.

>      if (this._controller === undefined) {
       throw new TypeError('This BYOB request has been invalidated');
     }
 
+    if (CanTransferArrayBuffer(view.buffer) === false) {

I think it's fine to change this back to `IsDetachedBuffer` too? We'll throw an error on [this line](https://github.com/whatwg/streams/blob/5da8d5c2bfdde4e3ce34ec3e63f4bd3f86922390/reference-implementation/lib/abstract-ops/readable-streams.js#L1339) instead, which is still *before* any state is mutated.

> @@ -994,6 +995,18 @@ function ReadableByteStreamControllerEnqueue(controller, chunk) {
     return;
   }
 
+  if (controller._pendingPullIntos.length > 0) {
+    const firstPendingPullInto = controller._pendingPullIntos[0];
+    if (CanTransferArrayBuffer(firstPendingPullInto.buffer) === false) {

We already know that the buffer of a pull-into descriptor has been transferred before when we constructed it, so this can be changed to `IsDetachedBuffer` too.

> @@ -1263,18 +1289,17 @@ function ReadableByteStreamControllerRespondInReadableState(controller, bytesWri
 
 function ReadableByteStreamControllerRespondInternal(controller, bytesWritten) {
   const firstDescriptor = controller._pendingPullIntos[0];
+  assert(CanTransferArrayBuffer(firstDescriptor.buffer) === true);

`IsDetachedBuffer` is sufficient here. Both `ReadableByteStreamControllerRespond` and `ReadableByteStreamControllerRespondWithNewView` will have called `TransferArrayBuffer` already.

> @@ -1283,24 +1308,42 @@ function ReadableByteStreamControllerRespondInternal(controller, bytesWritten) {
 
 function ReadableByteStreamControllerRespondWithNewView(controller, view) {
   assert(controller._pendingPullIntos.length > 0);
+  assert(CanTransferArrayBuffer(view.buffer) === true);

We'll change this back to `IsDetachedBuffer`. The `TransferArrayBuffer` call in this function happens before any state mutations, so it's fine.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/1123#pullrequestreview-661469378