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

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

cli/tests/unit/structured_clone_test.ts

@@ -27,4 +27,27 @@ Deno.test("correct DataCloneError message", () => {
     DOMException,
     "Value not transferable",
   );
+
+  const ab = new ArrayBuffer(1);
+  // detach ArrayBuffer
+  structuredClone(ab, { transfer: [ab] });
+  assertThrows(
+    () => {
+      structuredClone(ab, { transfer: [ab] });
+    },
+    DOMException,
+    "ArrayBuffer at index 0 is already detached",
+  );
+
+  const ab2 = new ArrayBuffer(0);
+  assertThrows(
+    () => {
+      structuredClone([ab2, ab], { transfer: [ab2, ab] });
+    },
+    DOMException,
+    "ArrayBuffer at index 1 is already detached",
+  );
+
+  // ab2 should not be detached after above failure
+  structuredClone(ab2, { transfer: [ab2] });
 });

core/ops_builtin_v8.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license.
 use crate::bindings::script_origin;
+use crate::error::custom_error;
 use crate::error::is_instance_of_error;
 use crate::error::range_error;
 use crate::error::type_error;
@@ -51,6 +52,7 @@ pub(crate) fn init_builtins_v8() -> Vec<OpDecl> {
     op_store_pending_promise_exception::decl(),
     op_remove_pending_promise_exception::decl(),
     op_has_pending_promise_exception::decl(),
+    op_arraybuffer_was_detached::decl(),
   ]
 }
 
@@ -448,22 +450,27 @@ fn op_serialize(
   if let Some(transferred_array_buffers) = transferred_array_buffers {
     let state_rc = JsRuntime::state(scope);
     let state = state_rc.borrow_mut();
-    for i in 0..transferred_array_buffers.length() {
-      let i = v8::Number::new(scope, i as f64).into();
+    for index in 0..transferred_array_buffers.length() {
+      let i = v8::Number::new(scope, index as f64).into();
       let buf = transferred_array_buffers.get(scope, i).unwrap();
       let buf = v8::Local::<v8::ArrayBuffer>::try_from(buf).map_err(|_| {
         type_error("item in transferredArrayBuffers not an ArrayBuffer")
       })?;
       if let Some(shared_array_buffer_store) = &state.shared_array_buffer_store
       {
-        // TODO(lucacasonato): we need to check here that the buffer is not
-        // already detached. We can not do that because V8 does not provide
-        // a way to check if a buffer is already detached.
         if !buf.is_detachable() {
           return Err(type_error(
             "item in transferredArrayBuffers is not transferable",
           ));
         }
+
+        if buf.was_detached() {
+          return Err(custom_error(
+            "DOMExceptionOperationError",
+            format!("ArrayBuffer at index {} is already detached", index),
+          ));
+        }
+
         let backing_store = buf.get_backing_store();
         buf.detach();
         let id = shared_array_buffer_store.insert(backing_store);
@@ -877,3 +884,12 @@ fn op_has_pending_promise_exception<'a>(
     .pending_promise_exceptions
     .contains_key(&promise_global)
 }
+
+#[op(v8)]
+fn op_arraybuffer_was_detached<'a>(
+  _scope: &mut v8::HandleScope<'a>,
+  input: serde_v8::Value<'a>,
+) -> Result<bool, Error> {
+  let ab = v8::Local::<v8::ArrayBuffer>::try_from(input.v8_value)?;
+  Ok(ab.was_detached())
+}

ext/web/06_streams.js

@@ -193,7 +193,13 @@
    * @returns {boolean}
    */
   function isDetachedBuffer(O) {
-    return ReflectHas(O, isFakeDetached);
+    if (O.byteLength !== 0) {
+      return false;
+    }
+    // TODO(marcosc90) remove isFakeDetached once transferArrayBuffer
+    // actually detaches the buffer
+    return ReflectHas(O, isFakeDetached) ||
+      core.ops.op_arraybuffer_was_detached(O);
   }
 
   /**

ext/web/13_message_port.js

@@ -26,9 +26,6 @@
     SymbolFor,
     SymbolIterator,
     TypeError,
-    WeakSet,
-    WeakSetPrototypeAdd,
-    WeakSetPrototypeHas,
   } = window.__bootstrap.primordials;
 
   class MessageChannel {
@@ -239,30 +236,25 @@
     return [data, transferables];
   }
 
-  const detachedArrayBuffers = new WeakSet();
-
   /**
    * @param {any} data
    * @param {object[]} transferables
    * @returns {globalThis.__bootstrap.messagePort.MessageData}
    */
   function serializeJsMessageData(data, transferables) {
-    const transferredArrayBuffers = ArrayPrototypeFilter(
-      transferables,
-      (a) => ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, a),
-    );
-
-    for (const arrayBuffer of transferredArrayBuffers) {
-      // This is hacky with both false positives and false negatives for
-      // detecting detached array buffers. V8  needs to add a way to tell if a
-      // buffer is detached or not.
-      if (WeakSetPrototypeHas(detachedArrayBuffers, arrayBuffer)) {
-        throw new DOMException(
-          "Can not transfer detached ArrayBuffer",
-          "DataCloneError",
-        );
+    const transferredArrayBuffers = [];
+    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);
       }
-      WeakSetPrototypeAdd(detachedArrayBuffers, arrayBuffer);
     }
 
     const serializedData = core.serialize(data, {