From 5c307a38dfa1aed07dbba69819ba764d303db762 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Fri, 8 Aug 2025 14:21:31 +0200 Subject: [PATCH] script_bindings Start wrapping unsafe code in `unsafe {}` (#38545) This is useful to better isolate `unsafe` code. Once all unsafe calls are wrapped we can enable the Rust warning. This also explicitly disables the warning for generated code, which is a much more difficult task. After this change there are 211 warnings left in `script_bindings`. Testing: This should not change behavior and is thus covered by existing tests. Signed-off-by: Martin Robinson --- components/script_bindings/callback.rs | 16 +++-- components/script_bindings/codegen/codegen.py | 21 ++++-- components/script_bindings/finalize.rs | 32 ++++++---- components/script_bindings/lib.rs | 2 +- components/script_bindings/mem.rs | 2 +- components/script_bindings/root.rs | 20 +++--- components/script_bindings/str.rs | 4 +- components/script_bindings/trace.rs | 64 ++++++++++--------- 8 files changed, 91 insertions(+), 70 deletions(-) diff --git a/components/script_bindings/callback.rs b/components/script_bindings/callback.rs index 2c43653c825..3e09541c60e 100644 --- a/components/script_bindings/callback.rs +++ b/components/script_bindings/callback.rs @@ -96,11 +96,13 @@ impl CallbackObject { unsafe fn init(&mut self, cx: JSContext, callback: *mut JSObject) { self.callback.set(callback); self.permanent_js_root.set(ObjectValue(callback)); - assert!(AddRawValueRoot( - *cx, - self.permanent_js_root.get_unsafe(), - b"CallbackObject::root\n".as_c_char_ptr() - )); + unsafe { + assert!(AddRawValueRoot( + *cx, + self.permanent_js_root.get_unsafe(), + b"CallbackObject::root\n".as_c_char_ptr() + )); + } } } @@ -173,7 +175,7 @@ impl CallbackFunction { /// # Safety /// `callback` must point to a valid, non-null JSObject. pub unsafe fn init(&mut self, cx: JSContext, callback: *mut JSObject) { - self.object.init(cx, callback); + unsafe { self.object.init(cx, callback) }; } } @@ -205,7 +207,7 @@ impl CallbackInterface { /// # Safety /// `callback` must point to a valid, non-null JSObject. pub unsafe fn init(&mut self, cx: JSContext, callback: *mut JSObject) { - self.object.init(cx, callback); + unsafe { self.object.init(cx, callback) }; } /// Returns the property with the given `name`, if it is a callable object, diff --git a/components/script_bindings/codegen/codegen.py b/components/script_bindings/codegen/codegen.py index 5e9a8400588..545124b2409 100644 --- a/components/script_bindings/codegen/codegen.py +++ b/components/script_bindings/codegen/codegen.py @@ -53,11 +53,22 @@ from configuration import ( ) AUTOGENERATED_WARNING_COMMENT = "/* THIS FILE IS AUTOGENERATED - DO NOT EDIT */\n\n" -ALLOWED_WARNING_LIST = ['non_camel_case_types', 'non_upper_case_globals', 'unused_imports', - 'unused_variables', 'unused_assignments', 'unused_mut', - 'clippy::approx_constant', 'clippy::enum_variant_names', 'clippy::let_unit_value', - 'clippy::needless_return', 'clippy::too_many_arguments', 'clippy::unnecessary_cast', - 'clippy::upper_case_acronyms'] +ALLOWED_WARNING_LIST = [ + 'non_camel_case_types', + 'non_upper_case_globals', + 'unsafe_op_in_unsafe_fn', + 'unused_imports', + 'unused_variables', + 'unused_assignments', + 'unused_mut', + 'clippy::approx_constant', + 'clippy::enum_variant_names', + 'clippy::let_unit_value', + 'clippy::needless_return', + 'clippy::too_many_arguments', + 'clippy::unnecessary_cast', + 'clippy::upper_case_acronyms' +] ALLOWED_WARNINGS = f"#![allow({','.join(ALLOWED_WARNING_LIST)})]\n\n" FINALIZE_HOOK_NAME = '_finalize' diff --git a/components/script_bindings/finalize.rs b/components/script_bindings/finalize.rs index 46fbc83c965..668e973f4bb 100644 --- a/components/script_bindings/finalize.rs +++ b/components/script_bindings/finalize.rs @@ -18,14 +18,16 @@ use crate::weakref::{DOM_WEAK_SLOT, WeakBox, WeakReferenceable}; /// Drop the resources held by reserved slots of a global object unsafe fn do_finalize_global(obj: *mut JSObject) { - let protolist = get_proto_or_iface_array(obj); - let list = (*protolist).as_mut_ptr(); - for idx in 0..PROTO_OR_IFACE_LENGTH as isize { - let entry = list.offset(idx); - let value = *entry; - <*mut JSObject>::post_barrier(entry, value, ptr::null_mut()); + unsafe { + let protolist = get_proto_or_iface_array(obj); + let list = (*protolist).as_mut_ptr(); + for idx in 0..PROTO_OR_IFACE_LENGTH as isize { + let entry = list.offset(idx); + let value = *entry; + <*mut JSObject>::post_barrier(entry, value, ptr::null_mut()); + } + let _: Box = Box::from_raw(protolist); } - let _: Box = Box::from_raw(protolist); } /// # Safety @@ -33,7 +35,7 @@ unsafe fn do_finalize_global(obj: *mut JSObject) { pub(crate) unsafe fn finalize_common(this: *const T) { if !this.is_null() { // The pointer can be null if the object is the unforgeable holder of that interface. - let _ = Box::from_raw(this as *mut T); + let _ = unsafe { Box::from_raw(this as *mut T) }; } debug!("{} finalize: {:p}", type_name::(), this); } @@ -42,8 +44,10 @@ pub(crate) unsafe fn finalize_common(this: *const T) { /// `obj` must point to a valid, non-null JS object. /// `this` must point to a valid, non-null instance of T. pub(crate) unsafe fn finalize_global(obj: *mut JSObject, this: *const T) { - do_finalize_global(obj); - finalize_common::(this); + unsafe { + do_finalize_global(obj); + finalize_common::(this); + } } /// # Safety @@ -54,11 +58,11 @@ pub(crate) unsafe fn finalize_weak_referenceable( this: *const T, ) { let mut slot = UndefinedValue(); - JS_GetReservedSlot(obj, DOM_WEAK_SLOT, &mut slot); + unsafe { JS_GetReservedSlot(obj, DOM_WEAK_SLOT, &mut slot) }; let weak_box_ptr = slot.to_private() as *mut WeakBox; if !weak_box_ptr.is_null() { let count = { - let weak_box = &*weak_box_ptr; + let weak_box = unsafe { &*weak_box_ptr }; assert!(weak_box.value.get().is_some()); assert!(weak_box.count.get() > 0); weak_box.value.set(None); @@ -67,8 +71,8 @@ pub(crate) unsafe fn finalize_weak_referenceable( count }; if count == 0 { - mem::drop(Box::from_raw(weak_box_ptr)); + mem::drop(unsafe { Box::from_raw(weak_box_ptr) }); } } - finalize_common::(this); + unsafe { finalize_common::(this) }; } diff --git a/components/script_bindings/lib.rs b/components/script_bindings/lib.rs index acd0e1978d2..b2ca824a633 100644 --- a/components/script_bindings/lib.rs +++ b/components/script_bindings/lib.rs @@ -45,7 +45,7 @@ pub mod trace; pub mod utils; pub mod weakref; -#[allow(non_snake_case)] +#[allow(non_snake_case, unsafe_op_in_unsafe_fn)] pub mod codegen { pub mod Globals { include!(concat!(env!("OUT_DIR"), "/Globals.rs")); diff --git a/components/script_bindings/mem.rs b/components/script_bindings/mem.rs index 52a6e724b22..605c1a8359e 100644 --- a/components/script_bindings/mem.rs +++ b/components/script_bindings/mem.rs @@ -16,5 +16,5 @@ pub(crate) unsafe fn malloc_size_of_including_raw_self( ops: &mut MallocSizeOfOps, obj: *const c_void, ) -> usize { - ops.malloc_size_of(obj) + (*(obj as *const T)).size_of(ops) + unsafe { ops.malloc_size_of(obj) + (*(obj as *const T)).size_of(ops) } } diff --git a/components/script_bindings/root.rs b/components/script_bindings/root.rs index f383447a997..1f635c4df83 100644 --- a/components/script_bindings/root.rs +++ b/components/script_bindings/root.rs @@ -40,13 +40,13 @@ where unsafe fn add_to_root_list(object: *const dyn JSTraceable) -> *const RootCollection { assert_in_script(); STACK_ROOTS.with(|root_list| { - let root_list = &*root_list.get().unwrap(); - root_list.root(object); + let root_list = unsafe { &*root_list.get().unwrap() }; + unsafe { root_list.root(object) }; root_list }) } - let root_list = add_to_root_list(value.stable_trace_object()); + let root_list = unsafe { add_to_root_list(value.stable_trace_object()) }; Root { value, root_list } } } @@ -79,7 +79,7 @@ where struct ReflectorStackRoot(Reflector); unsafe impl JSTraceable for ReflectorStackRoot { unsafe fn trace(&self, tracer: *mut JSTracer) { - trace_reflector(tracer, "on stack", &self.0); + unsafe { trace_reflector(tracer, "on stack", &self.0) }; } } unsafe { &*(self.reflector() as *const Reflector as *const ReflectorStackRoot) } @@ -102,9 +102,9 @@ where { unsafe fn trace(&self, tracer: *mut JSTracer) { if self.0.reflector().get_jsobject().is_null() { - self.0.trace(tracer); + unsafe { self.0.trace(tracer) }; } else { - trace_reflector(tracer, "on stack", self.0.reflector()); + unsafe { trace_reflector(tracer, "on stack", self.0.reflector()) }; } } } @@ -283,8 +283,8 @@ where pub unsafe fn reflect_with(self, obj: *mut JSObject) -> DomRoot { let ptr = self.as_ptr(); drop(self); - let root = DomRoot::from_ref(&*ptr); - root.init_reflector(obj); + let root = DomRoot::from_ref(unsafe { &*ptr }); + unsafe { root.init_reflector(obj) }; root } } @@ -400,13 +400,13 @@ impl RootCollection { /// Starts tracking a trace object. unsafe fn root(&self, object: *const dyn JSTraceable) { assert_in_script(); - (*self.roots.get()).push(object); + unsafe { (*self.roots.get()).push(object) }; } /// Stops tracking a trace object, asserting if it isn't found. unsafe fn unroot(&self, object: *const dyn JSTraceable) { assert_in_script(); - let roots = &mut *self.roots.get(); + let roots = unsafe { &mut *self.roots.get() }; match roots .iter() .rposition(|r| std::ptr::addr_eq(*r as *const (), object as *const ())) diff --git a/components/script_bindings/str.rs b/components/script_bindings/str.rs index 0ef6e0c528a..bf47a6531b6 100644 --- a/components/script_bindings/str.rs +++ b/components/script_bindings/str.rs @@ -321,8 +321,8 @@ pub fn serialize_jsval_to_json_utf8( data: *mut std::ffi::c_void, ) -> bool { let data = data as *mut ToJSONCallbackData; - let string_chars = slice::from_raw_parts(string, len as usize); - (*data) + let string_chars = unsafe { slice::from_raw_parts(string, len as usize) }; + unsafe { &mut *data } .string .get_or_insert_with(Default::default) .push_str(&String::from_utf16_lossy(string_chars)); diff --git a/components/script_bindings/trace.rs b/components/script_bindings/trace.rs index dbdf5be94df..cee852f84f6 100644 --- a/components/script_bindings/trace.rs +++ b/components/script_bindings/trace.rs @@ -106,14 +106,14 @@ pub unsafe trait CustomTraceable { unsafe impl CustomTraceable for Box { #[inline] unsafe fn trace(&self, trc: *mut JSTracer) { - (**self).trace(trc); + unsafe { (**self).trace(trc) }; } } unsafe impl CustomTraceable for OnceCell { unsafe fn trace(&self, tracer: *mut JSTracer) { if let Some(value) = self.get() { - value.trace(tracer) + unsafe { value.trace(tracer) } } } } @@ -124,13 +124,13 @@ unsafe impl CustomTraceable for Sender { unsafe impl CustomTraceable for ServoArc { unsafe fn trace(&self, trc: *mut JSTracer) { - (**self).trace(trc) + unsafe { (**self).trace(trc) } } } unsafe impl CustomTraceable for RwLock { unsafe fn trace(&self, trc: *mut JSTracer) { - self.read().trace(trc) + unsafe { self.read().trace(trc) } } } @@ -138,7 +138,7 @@ unsafe impl CustomTraceable for indexmap::IndexSet CustomTraceable for SmallVec<[T; 1]> { #[inline] unsafe fn trace(&self, trc: *mut JSTracer) { for e in self.iter() { - e.trace(trc); + unsafe { e.trace(trc) }; } } } @@ -163,8 +163,8 @@ where #[inline] unsafe fn trace(&self, trc: *mut JSTracer) { for (k, v) in self { - k.trace(trc); - v.trace(trc); + unsafe { k.trace(trc) }; + unsafe { v.trace(trc) }; } } } @@ -175,7 +175,7 @@ where { unsafe fn trace(&self, tracer: *mut JSTracer) { for (s, _origin) in self.iter() { - s.trace(tracer) + unsafe { s.trace(tracer) }; } } } @@ -186,7 +186,7 @@ where { unsafe fn trace(&self, tracer: *mut JSTracer) { for s in self.iter() { - s.trace(tracer) + unsafe { s.trace(tracer) }; } } } @@ -196,7 +196,7 @@ where S: JSTraceable + ::style::stylesheets::StylesheetInDocument + PartialEq + 'static, { unsafe fn trace(&self, tracer: *mut JSTracer) { - self.stylesheets.trace(tracer) + unsafe { self.stylesheets.trace(tracer) }; } } @@ -205,7 +205,7 @@ where Sink: JSTraceable + TendrilSink, { unsafe fn trace(&self, tracer: *mut JSTracer) { - self.inner_sink().trace(tracer); + unsafe { self.inner_sink().trace(tracer) }; } } @@ -228,15 +228,17 @@ where ref ring, ref little, } = *self; - wrist.trace(trc); - thumb_metacarpal.trace(trc); - thumb_phalanx_proximal.trace(trc); - thumb_phalanx_distal.trace(trc); - thumb_phalanx_tip.trace(trc); - index.trace(trc); - middle.trace(trc); - ring.trace(trc); - little.trace(trc); + unsafe { + wrist.trace(trc); + thumb_metacarpal.trace(trc); + thumb_phalanx_proximal.trace(trc); + thumb_phalanx_distal.trace(trc); + thumb_phalanx_tip.trace(trc); + index.trace(trc); + middle.trace(trc); + ring.trace(trc); + little.trace(trc); + } } } @@ -255,11 +257,13 @@ where ref phalanx_distal, ref phalanx_tip, } = *self; - metacarpal.trace(trc); - phalanx_proximal.trace(trc); - phalanx_intermediate.trace(trc); - phalanx_distal.trace(trc); - phalanx_tip.trace(trc); + unsafe { + metacarpal.trace(trc); + phalanx_proximal.trace(trc); + phalanx_intermediate.trace(trc); + phalanx_distal.trace(trc); + phalanx_tip.trace(trc); + } } } @@ -281,7 +285,7 @@ unsafe impl + JSTra } self.trace_handles(&tracer); - self.sink.trace(trc); + unsafe { self.sink.trace(trc) }; } } @@ -290,7 +294,7 @@ unsafe impl + Cust CustomTraceable for Tokenizer { unsafe fn trace(&self, trc: *mut JSTracer) { - self.sink.trace(trc); + unsafe { self.sink.trace(trc) }; } } @@ -314,7 +318,7 @@ unsafe impl(js::gc::RootedTraceableB unsafe impl JSTraceable for RootedTraceableBox { unsafe fn trace(&self, tracer: *mut JSTracer) { - self.0.trace(tracer); + unsafe { self.0.trace(tracer) }; } }