Various fixes for readable byte streams

[MattiasBuelens]

This started as a few fixes that we encountered while working on #1114, but it ended up being... a lot of fixes. 😅

• .respondWithNewView(newView) can now be called with a newView that is smaller than the BYOB request's view. This aligns it with .respond(bytesWritten), which also allows bytesWritten <= byobRequest.view.byteLength.
• .respondWithNewView() must now be called with an empty view when the stream is closed. This aligns it with .respond(bytesWritten), which requires bytesWritten to be 0 when closed.
• .respondWithNewView(newView) must now be called with a view whose view.buffer.byteLength matches that of the BYOB request. Ideally, we would like to assert that the new view's buffer is the "transferred version" of the BYOB request's buffer, but that's not possible yet with the tools currently provided by the ECMAScript specification.
• .enqueue(chunk) and .respondWithNewView(newView) now check that the given view's buffer is actually transferable. Previously, we only checked whether the buffer is not yet detached, but this is insufficient: a WebAssembly.Memory's buffer is never transferable. We also make sure to not transfer the given buffer until after we've checked all other preconditions, so the buffer is still intact if these methods were to throw an error.
• .enqueue() and .respond() now check that the BYOB request's view has not been transferred, since otherwise it's not possible to copy bytes into its buffer and/or transfer the buffer when committing. This could crash Chrome, but has already been fixed in https://crbug.com/1200302. The reproduction case (see below) is now covered by a WPT test.
• .enqueue(), .respond() and .respondWithNewView() immediately invalidate the BYOB request. Previously, we only did this if we actually filled the first pull-into descriptor, which doesn't always happen. (For example: if the pull-into descriptor's element size is 4, but we only have filled 1 or 2 bytes.)
• We now always transfer the pull-into descriptor's buffer when committing it (to fulfill a read request or read-into request). This is mainly a sanity check: the stream should never use this buffer after it has been committed.

Original description

While working on #1114, we found that byobRequest.respondWithNewView(newView) didn't support all the same use cases as a regular .respond(bytesWritten) call. In particular:

• newView was required to be the same length as the BYOB request's view. This would have required the underlying byte source to always fill the entire view. On the other hand, .respond() allows responding with fewer bytes up to the view's length, which is the desired behavior.
• When the stream is closed, .respond() can only be called with bytesWritten set to 0. To match this, .respondWithNewView() should only be called with an empty view in the closed state.

We now also require the view passed to .respondWithNewView(newView) to have the same capacity as the BYOB request's original buffer. Ideally, we would like to assert that the new view's buffer is the "transferred version" of the BYOB request's buffer, but that's not possible yet with the tools currently provided by the ECMAScript specification.

I also found cases where the stream would try to transfer an already transferred buffer. For example, this code crashes the tab in Chrome 92 (error code: STATUS_ACCESS_VIOLATION):

let rs = new ReadableStream({
    type: 'bytes',
    pull(c) {
        let buffer = c.byobRequest.view.buffer;
        // Detach the buffer.
        postMessage(buffer, '*', [buffer]);
        // Try to enqueue with a new buffer.
        // The stream will try to copy to the pull-into request's view, but it can't because it has lost the buffer.
        c.enqueue(new Uint8Array([42]));
    }
});
let reader = rs.getReader({ mode: 'byob' });
await reader.read(new Uint8Array(1));

I fixed this by requiring the BYOB request's buffer to not be detached when calling byteStreamController.enqueue() or .close().

• At least two implementers are interested (and none opposed):
  • Chrome (@ricea)
  • …
• Tests are written and can be reviewed and commented upon at:
  • Streams: tests for readable byte stream fixes web-platform-tests/wpt#28557
• Implementation bugs are filed:
  • Chrome: https://crbug.com/1223565
  • Firefox: …
  • Safari: …

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[MattiasBuelens]

I also noticed that ReadableByteStreamControllerRespondWithNewView can still throw an error after we've already updated the first pull-into descriptor's buffer to the new view's buffer. Specifically, ReadableByteStreamControllerRespondInternal may still throw a TypeError if bytesWritten is or is not zero (depending on whether we're in the readable or closed state).

I think we will need to turn these checks into asserts inside RespondInternal(), and throw errors in Respond() and RespondWithNewView()?

[MattiasBuelens]

Hmm, we should probably also tackle #1001 while we're at it. It's often not enough to check if IsDetachedBuffer returns false, we should actually try to transfer any user-provided ArrayBuffer as soon as possible and propagate any errors before we're adding/changing pull-into descriptors deep inside our abstract ops. 🤔

[domenic]

I think we will need to turn these checks into asserts inside RespondInternal(), and throw errors in Respond() and RespondWithNewView()?

That does sound nicer. It's possible our original reasoning was that in error states like these, there are no guarantees, but we might as well make it better.

[ricea]

I'm working on a fix for the Chrome crash that Mattias found, which I will land before the rest of these changes.

[ricea]

I am interested in this on behalf of Chrome.

Do you want me to file an issue for Chrome?

[MattiasBuelens]

Sure, go ahead. 🙂

Who do we need to ping for interest from other browsers?

Do we need to file bugs for other browsers? AFAIK readable byte streams are only implemented in Chrome at the moment, other browsers haven't implemented it yet.

[domenic]

IMO it's OK to treat bugfixes in readable byte streams as falling under the general precedent that other browsers have already expressed interest in readable byte streams; we don't need to block on them reviewing the fixes. (Although they're welcome to comment!) It's kind of a judgment call what's a bugfix vs. a normative change that would rise to the level of needing confirmation, but IMO these are all fine.

[MattiasBuelens]

FYI since I couldn't find a Chromium issue for this spec change yet, I've filed one myself: https://crbug.com/1223565

[ricea]

FYI since I couldn't find a Chromium issue for this spec change yet, I've filed one myself: https://crbug.com/1223565

Thanks! I dropped the ball there.