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 <mrobinson@igalia.com>

components/script_bindings/callback.rs

@@ -96,6 +96,7 @@ impl<D: DomTypes> CallbackObject<D> {
     unsafe fn init(&mut self, cx: JSContext, callback: *mut JSObject) {
         self.callback.set(callback);
         self.permanent_js_root.set(ObjectValue(callback));
+        unsafe {
             assert!(AddRawValueRoot(
                 *cx,
                 self.permanent_js_root.get_unsafe(),
@@ -103,6 +104,7 @@ impl<D: DomTypes> CallbackObject<D> {
             ));
         }
     }
+}
 
 impl<D: DomTypes> Drop for CallbackObject<D> {
     #[allow(unsafe_code)]
@@ -173,7 +175,7 @@ impl<D: DomTypes> CallbackFunction<D> {
     /// # 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<D: DomTypes> CallbackInterface<D> {
     /// # 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,

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',
+ALLOWED_WARNING_LIST = [
-                        'unused_variables', 'unused_assignments', 'unused_mut',
+    'non_camel_case_types',
-                        'clippy::approx_constant', 'clippy::enum_variant_names', 'clippy::let_unit_value',
+    'non_upper_case_globals',
-                        'clippy::needless_return', 'clippy::too_many_arguments', 'clippy::unnecessary_cast',
+    'unsafe_op_in_unsafe_fn',
-                        'clippy::upper_case_acronyms']
+    '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'

components/script_bindings/finalize.rs

@@ -18,6 +18,7 @@ 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) {
+    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 {
@@ -27,13 +28,14 @@ unsafe fn do_finalize_global(obj: *mut JSObject) {
         }
         let _: Box<ProtoOrIfaceArray> = Box::from_raw(protolist);
     }
+}
 
 /// # Safety
 /// `this` must point to a valid, non-null instance of T.
 pub(crate) unsafe fn finalize_common<T>(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::<T>(), this);
 }
@@ -42,9 +44,11 @@ pub(crate) unsafe fn finalize_common<T>(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<T>(obj: *mut JSObject, this: *const T) {
+    unsafe {
         do_finalize_global(obj);
         finalize_common::<T>(this);
     }
+}
 
 /// # Safety
 /// `obj` must point to a valid, non-null JS object.
@@ -54,11 +58,11 @@ pub(crate) unsafe fn finalize_weak_referenceable<T: WeakReferenceable>(
     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<T>;
     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<T: WeakReferenceable>(
             count
         };
         if count == 0 {
-            mem::drop(Box::from_raw(weak_box_ptr));
+            mem::drop(unsafe { Box::from_raw(weak_box_ptr) });
         }
     }
-    finalize_common::<T>(this);
+    unsafe { finalize_common::<T>(this) };
 }

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"));

components/script_bindings/mem.rs

@@ -16,5 +16,5 @@ pub(crate) unsafe fn malloc_size_of_including_raw_self<T: MallocSizeOf>(
     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) }
 }

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();
+                let root_list = unsafe { &*root_list.get().unwrap() };
-                root_list.root(object);
+                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<T> {
         let ptr = self.as_ptr();
         drop(self);
-        let root = DomRoot::from_ref(&*ptr);
+        let root = DomRoot::from_ref(unsafe { &*ptr });
-        root.init_reflector(obj);
+        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 ()))

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);
+        let string_chars = unsafe { slice::from_raw_parts(string, len as usize) };
-        (*data)
+        unsafe { &mut *data }
             .string
             .get_or_insert_with(Default::default)
             .push_str(&String::from_utf16_lossy(string_chars));

components/script_bindings/trace.rs

@@ -106,14 +106,14 @@ pub unsafe trait CustomTraceable {
 unsafe impl<T: CustomTraceable> CustomTraceable for Box<T> {
     #[inline]
     unsafe fn trace(&self, trc: *mut JSTracer) {
-        (**self).trace(trc);
+        unsafe { (**self).trace(trc) };
     }
 }
 
 unsafe impl<T: JSTraceable> CustomTraceable for OnceCell<T> {
     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<T> CustomTraceable for Sender<T> {
 
 unsafe impl<T: JSTraceable> CustomTraceable for ServoArc<T> {
     unsafe fn trace(&self, trc: *mut JSTracer) {
-        (**self).trace(trc)
+        unsafe { (**self).trace(trc) }
     }
 }
 
 unsafe impl<T: JSTraceable> CustomTraceable for RwLock<T> {
     unsafe fn trace(&self, trc: *mut JSTracer) {
-        self.read().trace(trc)
+        unsafe { self.read().trace(trc) }
     }
 }
 
@@ -138,7 +138,7 @@ unsafe impl<T: JSTraceable + Eq + Hash> CustomTraceable for indexmap::IndexSet<T
     #[inline]
     unsafe fn trace(&self, trc: *mut JSTracer) {
         for e in self.iter() {
-            e.trace(trc);
+            unsafe { e.trace(trc) };
         }
     }
 }
@@ -149,7 +149,7 @@ unsafe impl<T: JSTraceable + 'static> 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);
+            unsafe { k.trace(trc) };
-            v.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<UTF8>,
 {
     unsafe fn trace(&self, tracer: *mut JSTracer) {
-        self.inner_sink().trace(tracer);
+        unsafe { self.inner_sink().trace(tracer) };
     }
 }
 
@@ -228,6 +228,7 @@ where
             ref ring,
             ref little,
         } = *self;
+        unsafe {
             wrist.trace(trc);
             thumb_metacarpal.trace(trc);
             thumb_phalanx_proximal.trace(trc);
@@ -239,6 +240,7 @@ where
             little.trace(trc);
         }
     }
+}
 
 #[cfg(feature = "webxr")]
 unsafe impl<J> CustomTraceable for Finger<J>
@@ -255,6 +257,7 @@ where
             ref phalanx_distal,
             ref phalanx_tip,
         } = *self;
+        unsafe {
             metacarpal.trace(trc);
             phalanx_proximal.trace(trc);
             phalanx_intermediate.trace(trc);
@@ -262,6 +265,7 @@ where
             phalanx_tip.trace(trc);
         }
     }
+}
 
 unsafe impl<Handle: JSTraceable + Clone, Sink: TreeSink<Handle = Handle> + JSTraceable>
     CustomTraceable for TreeBuilder<Handle, Sink>
@@ -281,7 +285,7 @@ unsafe impl<Handle: JSTraceable + Clone, Sink: TreeSink<Handle = Handle> + JSTra
         }
 
         self.trace_handles(&tracer);
-        self.sink.trace(trc);
+        unsafe { self.sink.trace(trc) };
     }
 }
 
@@ -290,7 +294,7 @@ unsafe impl<Handle: JSTraceable + Clone, Sink: TokenSink<Handle = Handle> + Cust
     CustomTraceable for Tokenizer<Sink>
 {
     unsafe fn trace(&self, trc: *mut JSTracer) {
-        self.sink.trace(trc);
+        unsafe { self.sink.trace(trc) };
     }
 }
 
@@ -314,7 +318,7 @@ unsafe impl<Handle: JSTraceable + Clone, Sink: JSTraceable + XmlTreeSink<Handle
 
         let tree_builder = &self.sink;
         tree_builder.trace_handles(&tracer);
-        tree_builder.sink.trace(trc);
+        unsafe { tree_builder.sink.trace(trc) };
     }
 }
 
@@ -329,7 +333,7 @@ pub struct RootedTraceableBox<T: JSTraceable + 'static>(js::gc::RootedTraceableB
 
 unsafe impl<T: JSTraceable + 'static> JSTraceable for RootedTraceableBox<T> {
     unsafe fn trace(&self, tracer: *mut JSTracer) {
-        self.0.trace(tracer);
+        unsafe { self.0.trace(tracer) };
     }
 }