Throwing on detached buffers is not correct

[domenic]

647d873 added throwing on detached buffers, but this is not correct. See whatwg/webidl#352 and whatwg/webidl#151 .

Sigh. I guess I just implemented that from memory.

[domenic]

I'm not sure the best way to handle this. I see a few options:

1. Return a new empty array buffer (or corresponding view) when the incoming array buffer is detached. This is a bit tricky since then 99% of the time a non-throwing conversion returns the input, but sometimes it doesn't.

2. Revert to having no special detached handling. This is similar to returning a new empty buffer in practice, since V8 treats detached buffers as zero-length buffers in most parts of the API.

3. Implement the full " getting a reference to" / "getting a copy of" algorithms. This is not so great because specs generally don't use these algorithms yet and there's planned work to change them in https://www.w3.org/Bugs/Public/show_bug.cgi?id=28798.

4. Try to implement https://www.w3.org/Bugs/Public/show_bug.cgi?id=28798. This seems kind of reasonable. However, any implementation that actually performs a copy would add a performance hit to jsdom. And I'm not sure that's really necessary given that we're single-threaded anyway?

I'm leaning toward (2), and potentially reassessing if we run into an API where this actually matters.

[domenic]

Let's work through the options here in relation to the existing jsdom ecosystem.

The APIs we implement that take buffer source types as input are:

• crypto.getRandomValues()

  • spec: uses modern Web IDL "byte length" and "write" operations.
  • jsdom code: wants to directly write into the view using Node.js APIs. Should no-op on detached buffers. Not sure what it currently does.

• webSocket.send():

  • spec: somewhat unclear, see send() could be more precise whatwg/websockets#71. But probably wants to grab a copy of the bytes.
  • jsdom code: stores a reference to the bytes, so, probably has the incorrect behavior.

• new Blob():

  • spec: uses modern Web IDL "get a copy" operation.
  • jsdom code: uses Buffer.from(). Which I learned today apparently creates a copy for ArrayBuffers, but not for ArrayBufferViews. Oh dear. But it's OK-ish, because then we use Buffer.concat(), which will ensure a copy is made. Except... sometimes it might not, because it could use the internal buffer pool??

• xhr.send():

  • spec: uses modern Web IDL "get a copy" operation.
  • jsdom code: uses Buffer.from(). So, probably subject to mutations in the ArrayBufferView case only. Ugh.

• textDecoder.decode():

  • spec: uses modern Web IDL "get a copy" operation.
  • jsdom code: passes through to @exodus/bytes. This is fine, even for SharedArrayBuffers, since it is all single-threaded.

Conclusion: the specs are mostly using modern Web IDL operations these days. So, once those modern Web IDL operations get fixed, in whatwg/webidl#1529, we can just change jsdom to following them.

Concretely, something like the following would work well:

• Wait for Fix BufferSource algorithms for shared and resizable buffers whatwg/webidl#1529 to get merged.
• Fix this library's conversions to follow the spec, basically by removing all detached buffer handling.
• Implement some subset of the Web IDL operations in #js-buffer-source-types that would be useful for jsdom.
  • Probably the most convenient place to implement these is in this package, as a secondary file: require("webidl-conversions/buffer-source.js") or something like that?
  • Looking at the above, we definitely need getACopyOfBytesHeld(bufferSource). It should return a new fresh Uint8Array. It should completely avoid Buffer.from() and its shenanigans.
  • Do we need byteLength(bufferSource)? Not sure. The Web IDL spec will be quite complicated, but I think the JS implementation can simply be bufferSource.byteLength. Maybe a wrapper adds some clarity that we're following the spec exactly? Or maybe not.
  • I'm less sure if we should add writeIntoArrayBuffer(bytes, arrayBuffer) or writeIntoArrayBufferView(bytes, arrayBufferView, startingOffset = 0). The only use case for them above, crypto.getRandomValues(), is more-efficiently implemented using direct Node.js APIs. Probably, directly using the Node.js APIs like we're currently doing does not work right for detached buffers? But that would call for a different sort of implementation strategy.
  • The Streams reference implementation might need more operations.
• Update jsdom to use the new buffer source operations. This will require a bit of work to eradicate some internal Buffer usage in favor of internal Uint8Array usage. But that's a good idea anyway.
  • Try to write to-upstream WPTs for the latent buffer modification bugs while we do this.

/cc @MattiasBuelens who might already have been having thoughts along these lines.