Make encodeInto() throw when given a detached buffer

[annevk]

In whatwg/webidl#1385 we discovered some specification UB. However, that also raised the question as to whether this method should perhaps be throwing an exception when given a detached buffer.

I tend to think it should and hopefully there's no callers relying on it not throwing as it's still early days?

cc @hsivonen @inexorabletash @youennf

[inexorabletash]

cc @ricea

[ricea]

Chrome accidentally does nothing successfully.

I think we should throw. My guess is that no-one is intentionally using the current behaviour.

[domenic]

Is the goal to add a one-off check to Encoding, or to throw at the Web IDL layer? The latter would be kind of nice, but perhaps scarier.

[annevk]

Throwing at the IDL layer would change the exception for everything calling into StructuredSerializeWithTransfer at least. Not sure that's really possible. My idea was to add one-off checks and also to assert that the buffer is not detached in various IDL algorithms so callers are "forced" to deal with it.

[annevk]

This also applies to TextDecoder's decode() and looking at https://webidl.spec.whatwg.org/#dfn-get-buffer-source-copy the handling of detached buffers is pretty intentional at least for that and APIs like it.

I suspect it's all aligned with the original non-throwy view APIs. This makes me a bit more worried about introducing a throw here and there as it seems unlikely we will be able to be consistent.

Thoughts?

[skyz1]

I was made aware of this issue while fixing a crash in ladybird that was caused by passing a detached buffer to encodeInto. We now treat this case as a noop by reading and writing 0 bytes in order to align our implementation with the major engines.

In my opinion, throwing would have been the better solution, but this noop behavior is already widely adapted. Thus it would make sense to also explicitly include it in the spec.