Fix tee() incorrectly closing before enqueuing to the second branch

[MattiasBuelens]

I found a case where tee() inadvertently drops a chunk for the second branch, even though neither of the branches have been canceled. This is obviously very bad, because both branches should receive all enqueued chunks.

See web-platform-tests/wpt#31159 for the problematic test case. Here's how it breaks down for ReadableStreamDefaultTee:

1. We call controller.enqueue('a'). Since there is a pending read request from tee() on the original stream, we synchronously run its chunkSteps. However, this first calls queueMicrotask(), so the remainder will happen later.
2. We call controller.close(). Since we've already processed all pending read requests, the stream immediately moves to state = 'closed'.
3. One microtick passes. We run the remainder of chunkSteps.
   1. We set reading = false.
   2. We call ReadableStreamDefaultControllerEnqueue for branch1. Since there is a pending read request for branch1, it synchronously fulfills the request. And since we created both branches with the default HWM = 1, ReadableStreamDefaultControllerShouldCallPull is also true. So we call branch1's pullAlgorithm.
      1. We're back at the start of pullAlgorithm. However, we've already set reading = false, we do start a new read request on the original stream.
      2. But the original stream is already closed, so it synchronously runs closeSteps.
      3. This brings us back here. We immediately close both branches.
   3. But wait a second. We're still in chunkSteps. So we call ReadableStreamDefaultControllerEnqueue for branch2... except, it's already closed, so the chunk is ignored! 😱💥

This PR fixes it by keeping reading = true until after we've enqueued/responded to both branches. Afterwards, we check if one of the two branches tried to call pull() (through an extra readAgain flag), and then call pull() ourselves. When teeing a readable byte stream, we also have to keep track which of the two branches called pull(), so we can use the appropriate BYOB request if available.

To do:

• Update spec text to match reference implementation

---

• At least two implementers are interested (and none opposed):
  • …
  • …
• Tests are written and can be reviewed and commented upon at:
  • Streams: test whether enqueue() and close() works while both branches are pulling web-platform-tests/wpt#31159
• Implementation bugs are filed:
  • Chrome: N/A (already passes all new tests)
  • Firefox: N/A (already passes all new tests)
  • Safari: N/A (already passes all new tests)

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

---

Preview | Diff

[ricea]

Given the complexity of the problem, the solution is quite readable! IMHO this is ready for spec text.

[MattiasBuelens]

I'm getting a ton of link errors when building the spec text locally:

$ make local
bikeshed spec index.bs index.html --md-Text-Macro="COMMIT-SHA LOCAL COPY"
LINE ~350: No 'dfn' refs found for 'platform objects' that are marked for export.
LINK ERROR: No 'idl' refs found for 'unrestricted double'.
{{unrestricted double}}
LINK ERROR: No 'interface' refs found for 'object'.
<a data-link-type="interface" data-lt="object">object</a>
LINK ERROR: No 'interface' refs found for 'boolean'.
<a data-link-type="interface" data-lt="boolean">boolean</a>
LINK ERROR: No 'interface' refs found for 'Promise' with spec 'webidl'.
<a data-link-spec="webidl" data-link-type="interface" data-lt="Promise">Promise</a>
LINK ERROR: No 'interface' refs found for 'undefined'.
<a data-link-type="interface" data-lt="undefined">undefined</a>
...
LINE ~6947: No 'dfn' refs found for 'this' that are marked for export.
LINE ~6950: No 'dfn' refs found for 'this' that are marked for export.
LINE ~6955: No 'dfn' refs found for 'new'.
LINK ERROR: No 'dfn' refs found for 'constructor operation' that are marked for export.
[=constructor operation=]
 ✔  Successfully generated, with 439 linking errors

The PR build (make deploy) seems to run into the same problem. In the PR preview, all of the links like "Return a promise resolved with undefined" are broken. 😕

[ricea]

lgtm

Are there any other issues with the reentrancy here? Would it better to avoid the reentrancy by adding even more microtasks?

[MattiasBuelens]

Are there any other issues with the reentrancy here?

No, I don't think there are. Only Enqueue() and RespondWithNewView() can perform CallPullIfNeeded(), and those are only used inside the chunk steps. (Yes, there's a RespondWithNewView() inside the close steps of ReadableByteStreamTee, but that's after we've already called close() so it never does CallPullIfNeeded().)

Would it better to avoid the reentrancy by adding even more microtasks?

Nah, I prefer it this way. Adding more microtasks could make these sorts of bugs less obvious and even more difficult to debug. I also feel like the stream specification should try to do as much as possible within the same microtask, even if that means dealing with reentrancy.

[domenic]

LGTM, just don't forget to roll WPTs now that they're merged, and also file bugs on all three engines.

[MattiasBuelens]

Interestingly, all engines already pass the new test because they haven't yet implemented #1045. They still use a promise for ReadableStreamDefaultReaderRead, so they don't have the reentrancy problem with synchronously calling closeSteps yet.

So does it still make sense to file bugs? 🤔

[domenic]

Fascinating! In that case no need to file bugs, yeah. The test will ensure that they don't make the same mistake in their implementations that we did with the spec.

[MattiasBuelens]

Tests are currently failing because the latest WPT already contains the new tests for #1171. This PR should go green once the other PR has landed.