Repository: servo/servo (language: python)

Issue:
CryptoKey.algorithm should return a cached object
From https://w3c.github.io/webcrypto/#cryptokey-interface-members:
>Returns the [cached ECMAScript object](https://w3c.github.io/webcrypto/#concept-cached-object) associated with the [[[algorithm]]](https://w3c.github.io/webcrypto/#dfn-CryptoKey-slot-algorithm) internal slot.

Meanwhile, the implementation creates a new instance each time it's called: https://github.com/servo/servo/blob/main/components/script/dom/cryptokey.rs#L106-L127


Discussion / hints:
@jdm perhaps we can can introduce a way to cache the created object and checking it's existence before creating another one. We perhaps could first store the object associated with the ``` [[algorithm]]``` internal slot privately and return it whenever Algorithm is called. I am just thinking out loud, you can direct on this issue differently.
Actually specs clearly defines that for caching we use `_cached` slots: https://w3c.github.io/webcrypto/#concept-cached-object; and [cryptokey already contains such slots:  ..., [[algorithm]], [[algorithm_cached]] ,...](https://w3c.github.io/webcrypto/#dfn-CryptoKey-slot-algorithm).
As far as I know, the slots terminology is just way to represent a persistent value associated with a JS object. It doesn't imply a particular implementation strategy.
> @jdm perhaps we can can introduce a way to cache the created object and checking it's existence before creating another one. We perhaps could first store the object associated with the ` [[algorithm]]` internal slot privately and return it whenever Algorithm is called. I am just thinking out loud, you can direct on this issue differently.

Yes, this is similar to the MutNullableDom type we use in many places, that allows returning a previously created value or initializing it with a new one.
> As far as I know, the slots terminology is just way to represent a persistent value associated with a JS object. It doesn't imply a particular implementation strategy.

Indeed, but I usually follow spec exactly.

Fix patch:
diff --git a/components/script/dom/cryptokey.rs b/components/script/dom/cryptokey.rs
index 88c189feb81a3..cf0e287e518e0 100644
--- a/components/script/dom/cryptokey.rs
+++ b/components/script/dom/cryptokey.rs
@@ -6,13 +6,12 @@ use std::cell::Cell;
 use std::ptr::NonNull;
 
 use dom_struct::dom_struct;
-use js::jsapi::{JSObject, Value};
+use js::jsapi::{Heap, JSObject, Value};
+use js::rust::HandleObject;
 
-use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::codegen::Bindings::CryptoKeyBinding::{
     CryptoKeyMethods, KeyType, KeyUsage,
 };
-use crate::dom::bindings::codegen::Bindings::SubtleCryptoBinding::{AesKeyAlgorithm, KeyAlgorithm};
 use crate::dom::bindings::reflector::{reflect_dom_object, Reflector};
 use crate::dom::bindings::root::DomRoot;
 use crate::dom::bindings::str::DOMString;
@@ -22,6 +21,7 @@ use crate::script_runtime::JSContext;
 
 /// The underlying cryptographic data this key represents
 #[allow(dead_code)]
+#[derive(MallocSizeOf)]
 pub enum Handle {
     Aes128(Vec<u8>),
     Aes192(Vec<u8>),
@@ -32,13 +32,26 @@ pub enum Handle {
 #[dom_struct]
 pub struct CryptoKey {
     reflector_: Reflector,
+
+    /// <https://w3c.github.io/webcrypto/#dom-cryptokey-type>
     key_type: KeyType,
+
+    /// <https://w3c.github.io/webcrypto/#dom-cryptokey-extractable>
     extractable: Cell<bool>,
-    // This would normally be KeyAlgorithm but we cannot Send DOMString, which
-    // is a member of Algorithm
-    algorithm: DomRefCell<String>,
+
+    /// The name of the algorithm used
+    ///
+    /// This is always the same as the `name` of the
+    /// [`[[algorithm]]`](https://w3c.github.io/webcrypto/#dom-cryptokey-algorithm)
+    /// internal slot, but we store it here again for convenience
+    algorithm: DOMString,
+
+    /// <https://w3c.github.io/webcrypto/#dom-cryptokey-algorithm>
+    #[ignore_malloc_size_of = "Defined in mozjs"]
+    algorithm_object: Heap<*mut JSObject>,
+
+    /// <https://w3c.github.io/webcrypto/#dom-cryptokey-usages>
     usages: Vec<KeyUsage>,
-    #[ignore_malloc_size_of = "Defined in external cryptography crates"]
     #[no_trace]
     handle: Handle,
 }
@@ -47,15 +60,16 @@ impl CryptoKey {
     fn new_inherited(
         key_type: KeyType,
         extractable: bool,
-        algorithm: KeyAlgorithm,
         usages: Vec<KeyUsage>,
+        algorithm: DOMString,
         handle: Handle,
     ) -> CryptoKey {
         CryptoKey {
             reflector_: Reflector::new(),
             key_type,
             extractable: Cell::new(extractable),
-            algorithm: DomRefCell::new(algorithm.name.to_string()),
+            algorithm,
+            algorithm_object: Heap::default(),
             usages,
             handle,
         }
@@ -65,24 +79,29 @@ impl CryptoKey {
         global: &GlobalScope,
         key_type: KeyType,
         extractable: bool,
-        algorithm: KeyAlgorithm,
+        algorithm: DOMString,
+        algorithm_object: HandleObject,
         usages: Vec<KeyUsage>,
         handle: Handle,
     ) -> DomRoot<CryptoKey> {
-        reflect_dom_object(
+        let object = reflect_dom_object(
             Box::new(CryptoKey::new_inherited(
                 key_type,
                 extractable,
-                algorithm,
                 usages,
+                algorithm,
                 handle,
             )),
             global,
-        )
+        );
+
+        object.algorithm_object.set(algorithm_object.get());
+
+        object
     }
 
     pub fn algorithm(&self) -> String {
-        self.algorithm.borrow().to_string()
+        self.algorithm.to_string()
     }
 
     pub fn usages(&self) -> &[KeyUsage] {
@@ -105,31 +124,9 @@ impl CryptoKeyMethods for CryptoKey {
         self.extractable.get()
     }
 
-    #[allow(unsafe_code)]
     /// <https://w3c.github.io/webcrypto/#cryptokey-interface-members>
-    fn Algorithm(&self, cx: JSContext) -> NonNull<JSObject> {
-        let parent = KeyAlgorithm {
-            name: DOMString::from_string(self.algorithm()),
-        };
-        let algorithm = match self.handle() {
-            Handle::Aes128(_) => AesKeyAlgorithm {
-                parent,
-                length: 128,
-            },
-            Handle::Aes192(_) => AesKeyAlgorithm {
-                parent,
-                length: 192,
-            },
-            Handle::Aes256(_) => AesKeyAlgorithm {
-                parent,
-                length: 256,
-            },
-        };
-        unsafe {
-            rooted!(in(*cx) let mut alg: Value);
-            algorithm.to_jsval(*cx, alg.handle_mut());
-            NonNull::new(alg.to_object()).unwrap()
-        }
+    fn Algorithm(&self, _cx: JSContext) -> NonNull<JSObject> {
+        NonNull::new(self.algorithm_object.get()).unwrap()
     }
 
     #[allow(unsafe_code)]
diff --git a/components/script/dom/subtlecrypto.rs b/components/script/dom/subtlecrypto.rs
index 63088b8c7afc8..cb86981d30da3 100644
--- a/components/script/dom/subtlecrypto.rs
+++ b/components/script/dom/subtlecrypto.rs
@@ -12,7 +12,7 @@ use aes::{Aes128, Aes192, Aes256};
 use base64::prelude::*;
 use dom_struct::dom_struct;
 use js::conversions::ConversionResult;
-use js::jsapi::JSObject;
+use js::jsapi::{JSObject, JS_NewObject};
 use js::jsval::ObjectValue;
 use js::rust::MutableHandleObject;
 use js::typedarray::ArrayBufferU8;
@@ -25,8 +25,8 @@ use crate::dom::bindings::codegen::Bindings::CryptoKeyBinding::{
     CryptoKeyMethods, KeyType, KeyUsage,
 };
 use crate::dom::bindings::codegen::Bindings::SubtleCryptoBinding::{
-    AesCbcParams, AesCtrParams, AesKeyGenParams, Algorithm, AlgorithmIdentifier, JsonWebKey,
-    KeyAlgorithm, KeyFormat, SubtleCryptoMethods,
+    AesCbcParams, AesCtrParams, AesKeyAlgorithm, AesKeyGenParams, Algorithm, AlgorithmIdentifier,
+    JsonWebKey, KeyAlgorithm, KeyFormat, SubtleCryptoMethods,
 };
 use crate::dom::bindings::codegen::UnionTypes::{
     ArrayBufferViewOrArrayBuffer, ArrayBufferViewOrArrayBufferOrJsonWebKey,
@@ -796,6 +796,7 @@ impl SubtleCrypto {
 
     /// <https://w3c.github.io/webcrypto/#aes-cbc-operations>
     /// <https://w3c.github.io/webcrypto/#aes-ctr-operations>
+    #[allow(unsafe_code)]
     fn generate_key_aes(
         &self,
         usages: Vec<KeyUsage>,
@@ -827,25 +828,40 @@ impl SubtleCrypto {
             _ => return Err(Error::NotSupported),
         };
 
-        Ok(CryptoKey::new(
+        let cx = GlobalScope::get_cx();
+        rooted!(in(*cx) let mut algorithm_object = unsafe {JS_NewObject(*cx, ptr::null()) });
+        assert!(!algorithm_object.is_null());
+
+        AesKeyAlgorithm::from_name_and_size(
+            name.clone(),
+            key_gen_params.length,
+            algorithm_object.handle_mut(),
+            cx,
+        );
+
+        let crypto_key = CryptoKey::new(
             &self.global(),
             KeyType::Secret,
             extractable,
-            KeyAlgorithm { name },
+            name,
+            algorithm_object.handle(),
             usages,
             handle,
-        ))
+        );
+
+        Ok(crypto_key)
     }
 
     /// <https://w3c.github.io/webcrypto/#aes-cbc-operations>
     /// <https://w3c.github.io/webcrypto/#aes-ctr-operations>
+    #[allow(unsafe_code)]
     fn import_key_aes(
         &self,
         format: KeyFormat,
         data: &[u8],
         extractable: bool,
         usages: Vec<KeyUsage>,
-        alg: &str,
+        alg_name: &str,
     ) -> Result<DomRoot<CryptoKey>, Error> {
         if usages.iter().any(|usage| {
             !matches!(
@@ -865,15 +881,30 @@ impl SubtleCrypto {
             256 => Handle::Aes256(data.to_vec()),
             _ => return Err(Error::Data),
         };
-        let name = DOMString::from(alg);
-        Ok(CryptoKey::new(
+
+        let name = DOMString::from(alg_name.to_string());
+
+        let cx = GlobalScope::get_cx();
+        rooted!(in(*cx) let mut algorithm_object = unsafe {JS_NewObject(*cx, ptr::null()) });
+        assert!(!algorithm_object.is_null());
+
+        AesKeyAlgorithm::from_name_and_size(
+            name.clone(),
+            (data.len() * 8) as u16,
+            algorithm_object.handle_mut(),
+            cx,
+        );
+        let crypto_key = CryptoKey::new(
             &self.global(),
             KeyType::Secret,
             extractable,
-            KeyAlgorithm { name },
+            name,
+            algorithm_object.handle(),
             usages,
             handle,
-        ))
+        );
+
+        Ok(crypto_key)
     }
 
     /// <https://w3c.github.io/webcrypto/#aes-cbc-operations>
@@ -939,3 +970,19 @@ fn data_to_jwk_params(alg: &str, size: &str, key: &[u8]) -> (DOMString, DOMStrin
     data.retain(|c| c != '=');
     (jwk_alg, DOMString::from(data))
 }
+
+impl AesKeyAlgorithm {
+    /// Fill the object referenced by `out` with an [AesKeyAlgorithm]
+    /// of the specified name and size.
+    #[allow(unsafe_code)]
+    fn from_name_and_size(name: DOMString, size: u16, out: MutableHandleObject, cx: JSContext) {
+        let key_algorithm = Self {
+            parent: KeyAlgorithm { name: name.clone() },
+            length: size,
+        };
+
+        unsafe {
+            key_algorithm.to_jsobject(*cx, out);
+        }
+    }
+}


Test patch:
diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json
index 2933bbcf217a4..5b748d4c8a7fa 100644
--- a/tests/wpt/meta/MANIFEST.json
+++ b/tests/wpt/meta/MANIFEST.json
@@ -514372,6 +514372,31 @@
       }
      ]
     ],
+    "cryptokey_algorithm_returns_cached_object.https.any.js": [
+     "b2d73fbab78d64fa7e523adb579636d6295ee81a",
+     [
+      "WebCryptoAPI/cryptokey_algorithm_returns_cached_object.https.any.html",
+      {
+       "script_metadata": [
+        [
+         "title",
+         "WebCryptoAPI: CryptoKey.algorithm getter returns cached object"
+        ]
+       ]
+      }
+     ],
+     [
+      "WebCryptoAPI/cryptokey_algorithm_returns_cached_object.https.any.worker.html",
+      {
+       "script_metadata": [
+        [
+         "title",
+         "WebCryptoAPI: CryptoKey.algorithm getter returns cached object"
+        ]
+       ]
+      }
+     ]
+    ],
     "derive_bits_keys": {
      "cfrg_curves_bits_curve25519.https.any.js": [
       "866192e0193bc18953c224647ac51f2bb373746f",
diff --git a/tests/wpt/tests/WebCryptoAPI/cryptokey_algorithm_returns_cached_object.https.any.js b/tests/wpt/tests/WebCryptoAPI/cryptokey_algorithm_returns_cached_object.https.any.js
new file mode 100644
index 0000000000000..b2d73fbab78d6
--- /dev/null
+++ b/tests/wpt/tests/WebCryptoAPI/cryptokey_algorithm_returns_cached_object.https.any.js
@@ -0,0 +1,24 @@
+// META: title=WebCryptoAPI: CryptoKey.algorithm getter returns cached object
+
+// https://w3c.github.io/webcrypto/#dom-cryptokey-algorithm
+// https://github.com/servo/servo/issues/33908
+
+promise_test(function() {
+    return self.crypto.subtle.generateKey(
+        {
+          name: "AES-CTR",
+          length: 256,
+        },
+        true,
+        ["encrypt"],
+      ).then(
+        function(key) {
+          let a = key.algorithm;
+          let b = key.algorithm;
+          assert_true(a === b);
+        },
+        function(err) {
+            assert_unreached("generateKey threw an unexpected error: " + err.toString());
+        }
+    );
+}, "CryptoKey.algorithm getter returns cached object");
\ No newline at end of file