Access to reader.readIntoRequests without checking reader type

[rombel]

In ReadableByteStreamControllerRespondInClosedState [1], ReadableStreamGetNumReadIntoRequests is called and checks reader.readIntoRequests, but it is not checked that the reader is a BYOBReader.

This could be checked based on firstDescriptor.readerType, but the purpose of the loop (step 4) may be the processing of all pendingPullIntos objects, in which case I'm not sure the test that involves the number of readIntoRequests is the right one (I may not get all the context here).

[1] https://streams.spec.whatwg.org/#readable-byte-stream-controller-respond-in-closed-state

[domenic]

@tyoshino would you be able to take a look at this?

[domenic]

Hi @rombel, I'm sorry for the delay. Could you produce a bit of test code that you think might trigger this code path, i.e. a situation where ReadableByteStreamControllerRespondInClosedState is called, but the reader is a non-BYOB reader?

[rombel]

Hi @domenic, no problem for the delay. If I understand the spec correctly, below sample leads to calling ReadableByteStreamControllerRespondInClosedState with a default reader.

// First create a ReadableStream with a ReadableByteStreamController
// and a specific value for autoAllocateChunkSize.
let controller;
const rs = new ReadableStream({
	autoAllocateChunkSize: 128,
	start: function(c) {
		controller = c;
	},
	type: "bytes"
});

// As autoAllocateChunkSize is defined, calling read() leads to adding
// an item to controller.pendingPullIntos through controller pull() method.
rs.getReader().read();
// Hence, byobRequest can be obtained (step 2 of "get byobRequest").
const br = controller.byobRequest;
controller.close();
// Stream has been closed prior to calling respond() and bytesWritten value
// is 0: therefore, ReadableByteStreamControllerRespondInClosedState is called.
br.respond(0);

[domenic]

Thanks, I've confirmed this is indeed broken and am working on a fix!

[rombel]

Thanks!

[tyoshino]

Thanks rombel. Right. The purpose of the loop is to process all the entries.

As I said at #698 (comment), we need to review all the logic to make sure this functionality is correctly handled everywhere.