From 3ce95b2ba5693b0cca015b90c0938bf84ebe5d67 Mon Sep 17 00:00:00 2001 From: Jo Steven Novaryo <65610990+stevennovaryo@users.noreply.github.com> Date: Fri, 18 Jul 2025 03:32:36 +0800 Subject: [PATCH] script: Impl safe `from_jsval` wrapper (#38149) Implement `SafeFromJSValConvertible`, a safe wrapper for `ToJSValConvertible`. And, replace unsafe `ToJSValConvertible` with `SafeFromJSValConvertible` in `script/dom` to reduce the amount of unsafe code in `script`. This would support the implementation of `AdoptedStylesheet` where we will need to have a setter/getter of sequence, that was implemented by `any` types. Part of https://github.com/servo/servo/issues/37951 Signed-off-by: Jo Steven Novaryo --- components/script/dom/bindings/error.rs | 6 ++- .../script/dom/customelementregistry.rs | 34 +++++++---------- components/script/dom/readablestream.rs | 17 +++++---- components/script/dom/webxr/xrsystem.rs | 38 +++++++++---------- components/script/script_thread.rs | 24 ++++++------ components/script_bindings/conversions.rs | 24 ++++++++++++ 6 files changed, 79 insertions(+), 64 deletions(-) diff --git a/components/script/dom/bindings/error.rs b/components/script/dom/bindings/error.rs index 1fe3b187298..e280c347000 100644 --- a/components/script/dom/bindings/error.rs +++ b/components/script/dom/bindings/error.rs @@ -21,7 +21,9 @@ pub(crate) use script_bindings::error::*; #[cfg(feature = "js_backtrace")] use crate::dom::bindings::cell::DomRefCell; -use crate::dom::bindings::conversions::{ConversionResult, FromJSValConvertible, root_from_object}; +use crate::dom::bindings::conversions::{ + ConversionResult, SafeFromJSValConvertible, root_from_object, +}; use crate::dom::bindings::str::USVString; use crate::dom::domexception::{DOMErrorName, DOMException}; use crate::dom::globalscope::GlobalScope; @@ -208,7 +210,7 @@ impl ErrorInfo { } } - match unsafe { USVString::from_jsval(*cx, value, ()) } { + match USVString::safe_from_jsval(cx, value, ()) { Ok(ConversionResult::Success(USVString(string))) => ErrorInfo { message: format!("uncaught exception: {}", string), filename: String::new(), diff --git a/components/script/dom/customelementregistry.rs b/components/script/dom/customelementregistry.rs index 4dbe24a01a3..7c2b76086bf 100644 --- a/components/script/dom/customelementregistry.rs +++ b/components/script/dom/customelementregistry.rs @@ -15,7 +15,7 @@ use js::jsapi::{HandleValueArray, Heap, IsCallable, IsConstructor, JSAutoRealm, use js::jsval::{BooleanValue, JSVal, NullValue, ObjectValue, UndefinedValue}; use js::rust::wrappers::{Construct1, JS_GetProperty, SameValue}; use js::rust::{HandleObject, HandleValue, MutableHandleValue}; -use script_bindings::conversions::SafeToJSValConvertible; +use script_bindings::conversions::{SafeFromJSValConvertible, SafeToJSValConvertible}; use super::bindings::trace::HashMapTracedValues; use crate::dom::bindings::callback::{CallbackContainer, ExceptionHandling}; @@ -26,9 +26,7 @@ use crate::dom::bindings::codegen::Bindings::CustomElementRegistryBinding::{ use crate::dom::bindings::codegen::Bindings::ElementBinding::ElementMethods; use crate::dom::bindings::codegen::Bindings::FunctionBinding::Function; use crate::dom::bindings::codegen::Bindings::WindowBinding::Window_Binding::WindowMethods; -use crate::dom::bindings::conversions::{ - ConversionResult, FromJSValConvertible, StringificationBehavior, -}; +use crate::dom::bindings::conversions::{ConversionResult, StringificationBehavior}; use crate::dom::bindings::error::{ Error, ErrorResult, Fallible, report_pending_exception, throw_dom_exception, }; @@ -222,13 +220,11 @@ impl CustomElementRegistry { return Ok(Vec::new()); } - let conversion = unsafe { - FromJSValConvertible::from_jsval( - *cx, - observed_attributes.handle(), - StringificationBehavior::Default, - ) - }; + let conversion = SafeFromJSValConvertible::safe_from_jsval( + cx, + observed_attributes.handle(), + StringificationBehavior::Default, + ); match conversion { Ok(ConversionResult::Success(attributes)) => Ok(attributes), Ok(ConversionResult::Failure(error)) => Err(Error::Type(error.into())), @@ -258,7 +254,7 @@ impl CustomElementRegistry { } let conversion = - unsafe { FromJSValConvertible::from_jsval(*cx, form_associated_value.handle(), ()) }; + SafeFromJSValConvertible::safe_from_jsval(cx, form_associated_value.handle(), ()); match conversion { Ok(ConversionResult::Success(flag)) => Ok(flag), Ok(ConversionResult::Failure(error)) => Err(Error::Type(error.into())), @@ -287,13 +283,11 @@ impl CustomElementRegistry { return Ok(Vec::new()); } - let conversion = unsafe { - FromJSValConvertible::from_jsval( - *cx, - disabled_features.handle(), - StringificationBehavior::Default, - ) - }; + let conversion = SafeFromJSValConvertible::safe_from_jsval( + cx, + disabled_features.handle(), + StringificationBehavior::Default, + ); match conversion { Ok(ConversionResult::Success(attributes)) => Ok(attributes), Ok(ConversionResult::Failure(error)) => Err(Error::Type(error.into())), @@ -750,7 +744,7 @@ impl CustomElementDefinition { rooted!(in(*cx) let element_val = ObjectValue(element.get())); let element: DomRoot = - match unsafe { DomRoot::from_jsval(*cx, element_val.handle(), ()) } { + match SafeFromJSValConvertible::safe_from_jsval(cx, element_val.handle(), ()) { Ok(ConversionResult::Success(element)) => element, Ok(ConversionResult::Failure(..)) => { return Err(Error::Type( diff --git a/components/script/dom/readablestream.rs b/components/script/dom/readablestream.rs index c79ff8aacbb..0f5f236a76b 100644 --- a/components/script/dom/readablestream.rs +++ b/components/script/dom/readablestream.rs @@ -35,7 +35,7 @@ use crate::dom::abortsignal::{AbortAlgorithm, AbortSignal}; use crate::dom::bindings::codegen::Bindings::ReadableStreamDefaultReaderBinding::ReadableStreamDefaultReaderMethods; use crate::dom::bindings::codegen::Bindings::ReadableStreamDefaultControllerBinding::ReadableStreamDefaultController_Binding::ReadableStreamDefaultControllerMethods; use crate::dom::bindings::codegen::Bindings::UnderlyingSourceBinding::UnderlyingSource as JsUnderlyingSource; -use crate::dom::bindings::conversions::{ConversionBehavior, ConversionResult}; +use crate::dom::bindings::conversions::{ConversionBehavior, ConversionResult, SafeFromJSValConvertible}; use crate::dom::bindings::error::{Error, ErrorToJsval, Fallible}; use crate::dom::bindings::codegen::GenericBindings::WritableStreamDefaultWriterBinding::WritableStreamDefaultWriter_Binding::WritableStreamDefaultWriterMethods; use crate::dom::writablestream::WritableStream; @@ -58,7 +58,6 @@ use crate::dom::underlyingsourcecontainer::UnderlyingSourceType; use crate::dom::writablestreamdefaultwriter::WritableStreamDefaultWriter; use script_bindings::codegen::GenericBindings::MessagePortBinding::MessagePortMethods; use crate::dom::messageport::MessagePort; -use crate::js::conversions::FromJSValConvertible; use crate::realms::{enter_realm, InRealm}; use crate::script_runtime::{CanGc, JSContext as SafeJSContext}; use crate::dom::promisenativehandler::{Callback, PromiseNativeHandler}; @@ -2250,10 +2249,8 @@ pub(crate) unsafe fn get_type_and_value_from_message( .expect("Getting the value should not fail."); // Assert: type is a String. - let result = unsafe { - DOMString::from_jsval(*cx, type_.handle(), StringificationBehavior::Empty) - .expect("The type of the message should be a string") - }; + let result = DOMString::safe_from_jsval(cx, type_.handle(), StringificationBehavior::Empty) + .expect("The type of the message should be a string"); let ConversionResult::Success(type_string) = result else { unreachable!("The type of the message should be a string"); }; @@ -2357,7 +2354,7 @@ pub(crate) fn get_read_promise_done( rooted!(in(*cx) let object = v.to_object()); rooted!(in(*cx) let mut done = UndefinedValue()); match get_dictionary_property(*cx, object.handle(), "done", done.handle_mut(), can_gc) { - Ok(true) => match bool::from_jsval(*cx, done.handle(), ()) { + Ok(true) => match bool::safe_from_jsval(cx, done.handle(), ()) { Ok(ConversionResult::Success(val)) => Ok(val), Ok(ConversionResult::Failure(error)) => Err(Error::Type(error.to_string())), _ => Err(Error::Type("Unknown format for done property.".to_string())), @@ -2385,7 +2382,11 @@ pub(crate) fn get_read_promise_bytes( rooted!(in(*cx) let mut bytes = UndefinedValue()); match get_dictionary_property(*cx, object.handle(), "value", bytes.handle_mut(), can_gc) { Ok(true) => { - match Vec::::from_jsval(*cx, bytes.handle(), ConversionBehavior::EnforceRange) { + match Vec::::safe_from_jsval( + cx, + bytes.handle(), + ConversionBehavior::EnforceRange, + ) { Ok(ConversionResult::Success(val)) => Ok(val), Ok(ConversionResult::Failure(error)) => Err(Error::Type(error.to_string())), _ => Err(Error::Type("Unknown format for bytes read.".to_string())), diff --git a/components/script/dom/webxr/xrsystem.rs b/components/script/dom/webxr/xrsystem.rs index 9963d92fa59..f905982e88e 100644 --- a/components/script/dom/webxr/xrsystem.rs +++ b/components/script/dom/webxr/xrsystem.rs @@ -18,7 +18,7 @@ use crate::dom::bindings::cell::DomRefCell; use crate::dom::bindings::codegen::Bindings::XRSystemBinding::{ XRSessionInit, XRSessionMode, XRSystemMethods, }; -use crate::dom::bindings::conversions::{ConversionResult, FromJSValConvertible}; +use crate::dom::bindings::conversions::{ConversionResult, SafeFromJSValConvertible}; use crate::dom::bindings::error::Error; use crate::dom::bindings::inheritance::Castable; use crate::dom::bindings::refcounted::{Trusted, TrustedPromise}; @@ -192,33 +192,29 @@ impl XRSystemMethods for XRSystem { if let Some(ref r) = init.requiredFeatures { for feature in r { - unsafe { - if let Ok(ConversionResult::Success(s)) = - String::from_jsval(*cx, feature.handle(), ()) - { - required_features.push(s) - } else { - warn!("Unable to convert required feature to string"); - if mode != XRSessionMode::Inline { - self.pending_immersive_session.set(false); - } - promise.reject_error(Error::NotSupported, can_gc); - return promise; + if let Ok(ConversionResult::Success(s)) = + String::safe_from_jsval(cx, feature.handle(), ()) + { + required_features.push(s) + } else { + warn!("Unable to convert required feature to string"); + if mode != XRSessionMode::Inline { + self.pending_immersive_session.set(false); } + promise.reject_error(Error::NotSupported, can_gc); + return promise; } } } if let Some(ref o) = init.optionalFeatures { for feature in o { - unsafe { - if let Ok(ConversionResult::Success(s)) = - String::from_jsval(*cx, feature.handle(), ()) - { - optional_features.push(s) - } else { - warn!("Unable to convert optional feature to string"); - } + if let Ok(ConversionResult::Success(s)) = + String::safe_from_jsval(cx, feature.handle(), ()) + { + optional_features.push(s) + } else { + warn!("Unable to convert optional feature to string"); } } } diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index f25966a215f..91ab7bbabbc 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -107,7 +107,7 @@ use crate::dom::bindings::codegen::Bindings::DocumentBinding::{ use crate::dom::bindings::codegen::Bindings::NavigatorBinding::NavigatorMethods; use crate::dom::bindings::codegen::Bindings::WindowBinding::WindowMethods; use crate::dom::bindings::conversions::{ - ConversionResult, FromJSValConvertible, StringificationBehavior, + ConversionResult, SafeFromJSValConvertible, StringificationBehavior, }; use crate::dom::bindings::inheritance::Castable; use crate::dom::bindings::refcounted::Trusted; @@ -3681,18 +3681,16 @@ impl ScriptThread { ); load_data.js_eval_result = if jsval.get().is_string() { - unsafe { - let strval = DOMString::from_jsval( - *GlobalScope::get_cx(), - jsval.handle(), - StringificationBehavior::Empty, - ); - match strval { - Ok(ConversionResult::Success(s)) => { - Some(JsEvalResult::Ok(String::from(s).as_bytes().to_vec())) - }, - _ => None, - } + let strval = DOMString::safe_from_jsval( + GlobalScope::get_cx(), + jsval.handle(), + StringificationBehavior::Empty, + ); + match strval { + Ok(ConversionResult::Success(s)) => { + Some(JsEvalResult::Ok(String::from(s).as_bytes().to_vec())) + }, + _ => None, } } else { Some(JsEvalResult::NoContent) diff --git a/components/script_bindings/conversions.rs b/components/script_bindings/conversions.rs index 4c02775ff1f..37027e0709d 100644 --- a/components/script_bindings/conversions.rs +++ b/components/script_bindings/conversions.rs @@ -79,6 +79,30 @@ impl ToJSValConvertible for DOMString { } } +/// A safe wrapper for `FromJSValConvertible`. +pub trait SafeFromJSValConvertible: Sized { + type Config; + + #[allow(clippy::result_unit_err)] // Type definition depends on mozjs + fn safe_from_jsval( + cx: SafeJSContext, + value: HandleValue, + option: Self::Config, + ) -> Result, ()>; +} + +impl SafeFromJSValConvertible for T { + type Config = ::Config; + + fn safe_from_jsval( + cx: SafeJSContext, + value: HandleValue, + option: Self::Config, + ) -> Result, ()> { + unsafe { T::from_jsval(*cx, value, option) } + } +} + // https://heycam.github.io/webidl/#es-DOMString impl FromJSValConvertible for DOMString { type Config = StringificationBehavior;