From f58b0c6ad4f7c06088564434f2a4adc5b33206ec Mon Sep 17 00:00:00 2001 From: Euclid Ye Date: Thu, 8 May 2025 21:54:58 +0800 Subject: [PATCH] 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 --- components/script/webdriver_handlers.rs | 46 ++++++++++++------- .../execute_async_script/collections.py.ini | 3 -- .../classic/execute_script/collections.py.ini | 3 -- 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/components/script/webdriver_handlers.rs b/components/script/webdriver_handlers.rs index f90f3024a70..330ae4f0788 100644 --- a/components/script/webdriver_handlers.rs +++ b/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> for HashableJSVal { @@ -209,6 +209,7 @@ impl From> for HashableJSVal { } #[allow(unsafe_code)] +/// 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, ) -> 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::(cx, val) || is_arguments_object(cx, val) { + let return_val = if is_array_like::(cx, val) || + is_arguments_object(cx, val) + { let mut result: Vec = Vec::new(); let length = match get_property::( @@ -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::(*object, cx) { Ok(WebDriverJSValue::Element(WebElement( @@ -328,7 +334,7 @@ unsafe fn jsval_to_webdriver_inner( } else if let Ok(window) = root_from_object::(*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::().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) } diff --git a/tests/wpt/meta/webdriver/tests/classic/execute_async_script/collections.py.ini b/tests/wpt/meta/webdriver/tests/classic/execute_async_script/collections.py.ini index eb7c9dd30ae..710ae93d053 100644 --- a/tests/wpt/meta/webdriver/tests/classic/execute_async_script/collections.py.ini +++ b/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 diff --git a/tests/wpt/meta/webdriver/tests/classic/execute_script/collections.py.ini b/tests/wpt/meta/webdriver/tests/classic/execute_script/collections.py.ini index ff87e768065..710ae93d053 100644 --- a/tests/wpt/meta/webdriver/tests/classic/execute_script/collections.py.ini +++ b/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