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

@MattiasBuelens commented on this pull request.



> @@ -6081,6 +6101,17 @@ abstract operations are used to implement these "cross-realm transforms".
 
 The following abstract operations are a grab-bag of utilities.
 
+<div algorithm>
+ <dfn abstract-op lt="CanTransferArrayBuffer"
+ id="can-transfer-array-buffer">CanTransferArrayBuffer(|O|)</dfn> performs the following steps:
+
+ 1. Assert: [$Type$](|O|) is Object.
+ 1. Assert: |O| has an \[[ArrayBufferData]] internal slot.
+ 1. If ! [$IsDetachedBuffer$](|O|) is true, return false.
+ 1. If [$SameValue$](|O|.\[[ArrayBufferDetachKey]], undefined) is false, return false.

This is the important bit. `IsDetachedBuffer(O) === false` does not guarantee that `DetachArrayBuffer(O)` will not throw an error, since the buffer may be "non-detachable" (e.g. if it's a `WebAssembly.Memory`'s buffer). We have to perform the exact same steps as `DetachArrayBuffer(O)`, except for actually detaching the buffer.

> + 1. Let |state| be
+    |controller|.[=ReadableByteStreamController/[[stream]]=].[=ReadableStream/[[state]]=].
+ 1. If |state| is "`closed`",
+  1. If |view|.\[[ByteLength]] is not 0, throw a {{TypeError}} exception.
+ 1. Otherwise,
+  1. Assert: |state| is "`readable`".
+  1. If |view|.\[[ByteLength]] is 0, throw a {{TypeError}} exception.

These steps were moved from `RespondInternal()` to both `Respond()` and `RespondWithNewView()`. This way, we can throw errors *before* reaching this step in `RespondWithNewView()`:
> 10. Set *firstDescriptor*’s buffer to *view*.[[ViewedArrayBuffer]].

In `RespondInternal()`, these checks were turned into asserts.

> - 1. If [=this=].[=ReadableByteStreamController/[[pendingPullIntos]]=] is not [=list/is empty|empty=],
-  1. Let |firstDescriptor| be [=this=].[=ReadableByteStreamController/[[pendingPullIntos]]=][0].
-  1. If [$IsDetachedBuffer$](|firstDescriptor|'s [=pull-into descriptor/buffer=]) is true,
-     throw a {{TypeError}} exception.
- 1. Return ! [$ReadableByteStreamControllerEnqueue$]([=this=], |chunk|).
+ 1. Return ? [$ReadableByteStreamControllerEnqueue$]([=this=], |chunk|).

The transferable (previously: "not detached") check is moved to `ReadableByteStreamControllerEnqueue()`, so it can now throw.

Or would you prefer to keep the check in `ReadableByteStreamController.enqueue()`, and have `ReadableByteStreamControllerEnqueue()` assert that the pull-into descriptor's buffer is indeed still transferable?

> - 1. If [=this=].[=ReadableByteStreamController/[[pendingPullIntos]]=] is not [=list/is empty|empty=],
-  1. Let |firstDescriptor| be [=this=].[=ReadableByteStreamController/[[pendingPullIntos]]=][0].
-  1. If [$IsDetachedBuffer$](|firstDescriptor|'s [=pull-into descriptor/buffer=]) is true,
-     throw a {{TypeError}} exception.

[As mentioned here](https://github.com/whatwg/streams/pull/1123#discussion_r616209252), we need to ensure that we can call `close()` and still respond with a *transferred* view. Tests are in web-platform-tests/wpt@9b7f4358098f0aea35eb8de8333ec5b6b6f46446.

-- 
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-639347809