Discard auto-allocated BYOB request on enqueue

[MattiasBuelens]

As discovered in #1170 (comment): if a BYOB request was auto-allocated, but then you call controller.enqueue() instead of byobRequest.respond(), then the auto-allocated pull-into descriptor stays in the queue. If you later on switch from a default reader to a BYOB reader, you run into trouble because the list of read-into requests no longer matches up with the list of pending pull-into descriptors.

This PR fixes it by making enqueue() discard that auto-allocated BYOB request.

• At least two implementers are interested (and none opposed):
  • Chrome (@ricea)
  • …
• Tests are written and can be reviewed and commented upon at:
  • Streams: test whether enqueue() discards the auto-allocated BYOB request web-platform-tests/wpt#31128
• Implementation bugs are filed:
  • Chrome: https://crbug.com/1257465
  • Firefox: N/A (does not yet support readable byte streams)
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=231628

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

---

Preview | Diff

[ricea]

lgtm

[ricea]

I had to think about this for a while to understand why it was true. Could you perhaps add a note explaining it?

[MattiasBuelens]

Hang on... this is wrong. Every call to ReadableStreamDefaultReader.read() creates a new pull-into descriptor, regardless of whether we can pull() again. For example, this creates two such descriptors:

let controller;
const stream = new ReadableStream({
  start(c) {
    controller = c;
  },
  type: 'bytes',
  autoAllocateChunkSize: 16
}, {
  highWaterMark: 0
});

const reader = stream.getReader();
const read1 = reader.read();
const read2 = reader.read();

What is true is that, if autoAllocateChunkSize is not undefined, then the length of [[pendingPullIntos]] matches that of [[readRequests]]. So we need to remove only the first descriptor on enqueue().

[ricea]

Chrome is interested in this (I'm in the middle of implementing it).

[ricea]

Implementation bug for Chrome: https://crbug.com/1257465.

[MattiasBuelens]

@ricea I'm afraid you're going to have to re-open that Chromium bug. We assumed that there'd only be one auto-allocated pull-into descriptor, but that's not correct.

[ricea]

@ricea I'm afraid you're going to have to re-open that Chromium bug. We assumed that there'd only be one auto-allocated pull-into descriptor, but that's not correct.

Thanks for looking at it again! I am updating the fix in Chromium.

[domenic]

Tests are merged so you can roll them and then we can merge this PR.

Can you also file a WebKit bug? They appear to have a BYOB stream implementation on their main branch, at least. Firefox is still missing byte stream support.

[MattiasBuelens]

Implementation bugs filed.

Do we need to ask for implementer interest from the WebKit folks? Since this is more of a bug fix, I guess it's not that critical.

[domenic]

Yeah, for a bug fix I'm OK just merging as-is.