Streams: tests for readable byte stream fixes

[MattiasBuelens]

See whatwg/streams#1123 for the accompanying spec change.

To do:

• Test whether passing a view on a WebAssembly.Memory's buffer to byobReader.read(), byteStreamController.enqueue() and byobRequest.respondWithNewView() throws an error.

[MattiasBuelens]

This was the "easiest" solution I could come up with to transfer an ArrayBufferView.

We could also use postMessage() like we do with createTransferredReadableStream, but then we also need to change the test setup for the reference implementation... and I can't be bothered. 😛

[domenic]

enqueue-with-detached-buffer.window.js also uses postMessage, hmm. It might be worth changing https://github.com/whatwg/streams/blob/0ff6d4508b58acce28386696b2854b2a1570d205/reference-implementation/run-web-platform-tests.js#L43-L56 to just stick the "transferred" flag onto the transferred argument.

[MattiasBuelens]

You mean replacing window.postMessage(message, targetOrigin, transfer) to add our special "transferred" flag to every ArrayBuffer in transfer? It'll be a bit complicated:

• We need to export the TransferArrayBuffer helper in index.js, so that we can access it after loading the bundle. We have to use the same helper, so that it'll use the exact same isFakeDetached symbol.
• We have to transfer any ArrayBuffers inside transfer and replace all references to those buffers inside message. Sure, we can assume that for our tests, the message will always be an ArrayBufferView (and not some deeply nested JavaScript object). We'll then have to re-construct that view onto the transferred buffer.

Sure, I guess I can try it. Not sure if it'll be pretty though. 😛 (It's times like these where I wish tc39/proposal-resizablearraybuffer were already a reality... 😞)

[domenic]

To be clear, I'm not sure it's worth it; just an idea to throw out there!

[MattiasBuelens]

All right, so this would work:

const windowPostMessage = window.postMessage.bind(window);
window.postMessage = function (message, targetOrigin, transfer) {
  if (targetOrigin === '*' && transfer.length === 1 && transfer[0] instanceof window.ArrayBuffer) {
    const originalBuffer = transfer[0];
    const transferredBuffer = window.whatwgStreamsInternals.TransferArrayBuffer(originalBuffer);
    if (window.ArrayBuffer.isView(message) && message.buffer === originalBuffer) {
      message = new message.constructor(transferredBuffer, message.byteOffset, message.byteLength);
    }
    transfer = [transferredBuffer];
  }
  windowPostMessage(message, targetOrigin, transfer);
};

window.whatwgStreamsInternals would be defined in lib/index.js as:

// ...
const { TransferArrayBuffer } = require('./abstract-ops/ecmascript.js');
window.whatwgStreamsInternals = { TransferArrayBuffer };

Then we can change transferArrayBufferView inside the test utilities:

function transferArrayBufferView(view) {
  return new Promise((resolve) => {
    self.addEventListener('message', (event) => resolve(event.data), { once: true });
    self.postMessage(view, '*', [view.buffer]);
  });
}

Do we like this better? I think it looks nicer in WPT, but the runner becomes a little bit more complicated. That said, many different browser developers need to read and understand the WPT tests, so I suppose we should favor simplicity of the tests over simplicity of our test runner.

[ricea]

I prefer the version that doesn't involve us monkey patching postMessage in the reference implementation. The weirdness of the byte-stream-based transferArrayBufferView() implementation is nicely encapsulated, so it's not hurting anyone.

[domenic]

I slightly prefer the monkeypatch, but I'll defer to @ricea.

[domenic]

This seems like it's not testing the wasm memory case? Should we add those? It probably causes some reference implementation headaches, but seems like important coverage since those kind of un-transferrable buffers are so different from already-transferred ones.

[domenic]

enqueue-with-detached-buffer.window.js also uses postMessage, hmm. It might be worth changing https://github.com/whatwg/streams/blob/0ff6d4508b58acce28386696b2854b2a1570d205/reference-implementation/run-web-platform-tests.js#L43-L56 to just stick the "transferred" flag onto the transferred argument.

[MattiasBuelens]

Indeed, I didn't write tests for WebAssembly memories yet because I don't know how to make those work in the reference implementation... 😕

Is there an easy way to test whether a given ArrayBuffer belongs to a WebAssembly.Memory instance? I guess we could install a Proxy on WebAssembly.Memory.prototype.buffer and put all returned buffers inside a WeakSet. But then the reference implementation's TransferArrayBuffer (or CanTransferArrayBuffer) needs to know about that WeakSet somehow? 🤷

Alternatively, we put all the WebAssembly-related tests in a separate file and filter them out for the reference implementation's test runs?

[domenic]

Yeah, separate file is probably the best way to go.

[ricea]

Great! lgtm.