feat(ext/web): use ArrayBuffer.was_detached()

[marcosc90]

This PR adds a way to reliably check if an ArrayBuffer was detached

Blocked by: denoland/rusty_v8#1103

[andreubotella]

Why not make this a function in Deno.core, like Deno.core.isProxy?

[marcosc90]

Like this?

deno/core/01_core.js

Line 355 in edaba4d

isProxy: (value) => ops.op_is_proxy(value),

[andreubotella]

Yeah. The op would have to be moved to core/ops_builtin_v8.rs.

[bartlomieju]

@marcosc90 you should be now unblocked on this PR after rebasing against main

[marcosc90]

This can be changed after is merged #16294 to:

return core.ops.op_arraybuffer_was_detached(O);

[bartlomieju]

This is not needed, since you're calling it like core.ops.op_arraybuffer_was_detached in code. Feel free to remove it 👍

[marcosc90]

Done! I added it because it was proposed by Andreu #16307 (comment)

To be honest, I'm not sure what's the reason for exposing some methods to Deno.core if almost all of them are available in Deno.core.ops, couldn't find any info about the reasoning, AFAIK those APIs are not documented, although they're publicly exposed.

[bartlomieju]

It's legacy stuff, that was left to not break deno_std. We should really fix this...

[marcosc90]

Good to know, thanks!

[lucacasonato]

Suggested change

	for (let i = 0, j = 0; i < transferables.length; i++) {
	const ab = transferables[i];
	if (ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, ab)) {
	if (ab.byteLength === 0 && core.ops.op_arraybuffer_was_detached(ab)) {
	throw new DOMException(
	`ArrayBuffer at index ${j} is already detached`,
	"DataCloneError",
	);
	}
	j++;
	transferredArrayBuffers.push(ab);
	for (let i = 0; i < transferables.length; i++) {
	const ab = transferables[i];
	if (ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, ab)) {
	if (ab.byteLength === 0 && core.ops.op_arraybuffer_was_detached(ab)) {
	throw new DOMException(
	`ArrayBuffer at index ${i} is already detached`,
	"DataCloneError",
	);
	}
	transferredArrayBuffers.push(ab);

The user doesn't care about the index of the AB, they care about the index in the transferables array.

[marcosc90]

@lucacasonato Wanted to mimic Chrome error message.

const buf = new Uint8Array(5);
structuredClone(buf, { transfer: [buf.buffer] });
structuredClone([buf.buffer], { transfer: [new ReadableStream(), buf.buffer] });
// Uncaught DOMException: ... ArrayBuffer at index 0 is already detached

Also did it that way to make JS error be the same as Rust's where we're looping through AB list, not transferables

deno/core/ops_builtin_v8.rs

Line 471 in d361155

format!("ArrayBuffer at index {} is already detached", index),

Since we're not passing the transferables array to Rust, should I just remove the index hint, and leave the old message: Can not transfer detached ArrayBuffer?

[bartlomieju]

Okay, I think mimicing Chrome is fine here.

[bartlomieju]

LGTM, thanks @marcosc90!