rework webdriver deserialization to avoid false-positive cycle error (#36908)

1. Avoid false-positive cycle error when deserilizing
 - Only detect cycle for Objects
  - Remove last element of seen when success
2. Cite spec

Testing: `./mach test-wpt --product servodriver -r
tests\wpt\tests\webdriver\tests\classic\element_click\events.py`
Fixes: #36890 
cc @jdm @xiaochengh

---------

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

components/script/webdriver_handlers.rs

@@ -199,7 +199,7 @@ unsafe fn is_arguments_object(cx: *mut JSContext, value: HandleValue) -> bool {
     jsstring_to_str(cx, class_name) == "[object Arguments]"
 }
 
-#[derive(Eq, Hash, PartialEq)]
+#[derive(Clone, Eq, Hash, PartialEq)]
 struct HashableJSVal(u64);
 
 impl From<HandleValue<'_>> for HashableJSVal {
@@ -209,6 +209,7 @@ impl From<HandleValue<'_>> for HashableJSVal {
 }
 
 #[allow(unsafe_code)]
+/// <https://w3c.github.io/webdriver/#dfn-json-deserialize>
 pub(crate) fn jsval_to_webdriver(
     cx: SafeJSContext,
     global_scope: &GlobalScope,
@@ -231,12 +232,6 @@ unsafe fn jsval_to_webdriver_inner(
     val: HandleValue,
     seen: &mut HashSet<HashableJSVal>,
 ) -> WebDriverJSResult {
-    let hashable = val.into();
-    if seen.contains(&hashable) {
-        return Err(WebDriverJSError::JSError);
-    }
-    seen.insert(hashable);
-
     let _ac = enter_realm(global_scope);
     if val.get().is_undefined() {
         Ok(WebDriverJSValue::Undefined)
@@ -268,14 +263,26 @@ unsafe fn jsval_to_webdriver_inner(
                 _ => unreachable!(),
             };
         Ok(WebDriverJSValue::String(String::from(string)))
-    } else if val.get().is_object() {
+    }
+    // https://w3c.github.io/webdriver/#dfn-clone-an-object
+    else if val.get().is_object() {
+        let hashable = val.into();
+        // Step 1. If value is in `seen`, return error with error code javascript error.
+        if seen.contains(&hashable) {
+            return Err(WebDriverJSError::JSError);
+        }
+        //Step 2. Append value to `seen`.
+        seen.insert(hashable.clone());
+
         rooted!(in(cx) let object = match FromJSValConvertible::from_jsval(cx, val, ()).unwrap() {
             ConversionResult::Success(object) => object,
             _ => unreachable!(),
         });
         let _ac = JSAutoRealm::new(cx, *object);
 
-        if is_array_like::<crate::DomTypeHolder>(cx, val) || is_arguments_object(cx, val) {
+        let return_val = if is_array_like::<crate::DomTypeHolder>(cx, val) ||
+            is_arguments_object(cx, val)
+        {
             let mut result: Vec<WebDriverJSValue> = Vec::new();
 
             let length = match get_property::<u32>(
@@ -298,7 +305,7 @@ unsafe fn jsval_to_webdriver_inner(
                     return Err(WebDriverJSError::JSError);
                 },
             };
-
+            // Step 4. For each enumerable property in value, run the following substeps:
             for i in 0..length {
                 rooted!(in(cx) let mut item = UndefinedValue());
                 match get_property_jsval(cx, object.handle(), &i.to_string(), item.handle_mut()) {
@@ -319,7 +326,6 @@ unsafe fn jsval_to_webdriver_inner(
                     },
                 }
             }
-
             Ok(WebDriverJSValue::ArrayLike(result))
         } else if let Ok(element) = root_from_object::<Element>(*object, cx) {
             Ok(WebDriverJSValue::Element(WebElement(
@@ -328,7 +334,7 @@ unsafe fn jsval_to_webdriver_inner(
         } else if let Ok(window) = root_from_object::<Window>(*object, cx) {
             let window_proxy = window.window_proxy();
             if window_proxy.is_browsing_context_discarded() {
-                Err(WebDriverJSError::StaleElementReference)
+                return Err(WebDriverJSError::StaleElementReference);
             } else if window_proxy.browsing_context_id() == window_proxy.webview_id() {
                 Ok(WebDriverJSValue::Window(WebWindow(
                     window.Document().upcast::<Node>().unique_id(),
@@ -348,7 +354,12 @@ unsafe fn jsval_to_webdriver_inner(
                 &HandleValueArray::empty(),
                 value.handle_mut(),
             ) {
-                jsval_to_webdriver_inner(cx, global_scope, value.handle(), seen)
+                Ok(jsval_to_webdriver_inner(
+                    cx,
+                    global_scope,
+                    value.handle(),
+                    seen,
+                )?)
             } else {
                 throw_dom_exception(
                     SafeJSContext::from_ptr(cx),
@@ -356,7 +367,7 @@ unsafe fn jsval_to_webdriver_inner(
                     Error::JSFailed,
                     CanGc::note(),
                 );
-                Err(WebDriverJSError::JSError)
+                return Err(WebDriverJSError::JSError);
             }
         } else {
             let mut result = HashMap::new();
@@ -408,9 +419,12 @@ unsafe fn jsval_to_webdriver_inner(
                     }
                 }
             }
-
             Ok(WebDriverJSValue::Object(result))
-        }
+        };
+        // Step 5. Remove the last element of `seen`.
+        seen.remove(&hashable);
+        // Step 6. Return success with data `result`.
+        return_val
     } else {
         Err(WebDriverJSError::UnknownType)
     }

tests/wpt/meta/webdriver/tests/classic/execute_async_script/collections.py.ini

@@ -1,7 +1,4 @@
 [collections.py]
-  [test_array_in_array]
-    expected: FAIL
-
   [test_file_list]
     expected: FAIL
 

tests/wpt/meta/webdriver/tests/classic/execute_script/collections.py.ini

@@ -4,6 +4,3 @@
 
   [test_html_all_collection]
     expected: FAIL
-
-  [test_array_in_array]
-    expected: FAIL