From 2366a8bf9e0aab6758bb6f199f1ee06e1e1490f8 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Fri, 11 Jul 2025 13:38:02 +0200 Subject: [PATCH] script: Wrapping unsafe code in `unsafe` blocks for basic DOM types (#37997) There is a new default cargo clippy lint, `unsafe_op_in_unsafe_fn`, which requires unsafe code to be wrapped in unsafe blocks, even inside functions marked as unsafe. The lint is disabled as much of our code doesn't fulfill this contract. The thing itself is pretty useful in order to gradually remove unsafety, so this change starts adding `unsafe` blocks so we can eventually enable this lint. Testing: This doesn't change behavior so existings tests should suffice. Fixes: This is part of #35955. Signed-off-by: Martin Robinson --- components/script/dom/node.rs | 16 +++++--- components/script/dom/window.rs | 2 +- components/script/layout_dom/element.rs | 42 +++++++++++++-------- components/script/layout_dom/node.rs | 13 ++++--- components/script/layout_dom/shadow_root.rs | 6 ++- components/script/script_thread.rs | 2 +- 6 files changed, 51 insertions(+), 30 deletions(-) diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 067b8f0b4c9..f0e490cf495 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1600,7 +1600,8 @@ where /// returns it. #[allow(unsafe_code)] pub(crate) unsafe fn from_untrusted_node_address(candidate: UntrustedNodeAddress) -> DomRoot { - DomRoot::from_ref(Node::from_untrusted_node_address(candidate)) + let node = unsafe { Node::from_untrusted_node_address(candidate) }; + DomRoot::from_ref(node) } #[allow(unsafe_code)] @@ -1806,7 +1807,7 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> { #[inline] #[allow(unsafe_code)] unsafe fn initialize_style_data(self) { - let data = self.unsafe_get().style_data.borrow_mut_for_layout(); + let data = unsafe { self.unsafe_get().style_data.borrow_mut_for_layout() }; debug_assert!(data.is_none()); *data = Some(Box::default()); } @@ -1814,7 +1815,7 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> { #[inline] #[allow(unsafe_code)] unsafe fn initialize_layout_data(self, new_data: Box) { - let data = self.unsafe_get().layout_data.borrow_mut_for_layout(); + let data = unsafe { self.unsafe_get().layout_data.borrow_mut_for_layout() }; debug_assert!(data.is_none()); *data = Some(new_data); } @@ -1822,8 +1823,10 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> { #[inline] #[allow(unsafe_code)] unsafe fn clear_style_and_layout_data(self) { - self.unsafe_get().style_data.borrow_mut_for_layout().take(); - self.unsafe_get().layout_data.borrow_mut_for_layout().take(); + unsafe { + self.unsafe_get().style_data.borrow_mut_for_layout().take(); + self.unsafe_get().layout_data.borrow_mut_for_layout().take(); + } } fn is_text_input(&self) -> bool { @@ -3054,7 +3057,8 @@ impl Node { if object.is_null() { panic!("Attempted to create a `Node` from an invalid pointer!") } - &*(conversions::private_from_object(object) as *const Self) + + unsafe { &*(conversions::private_from_object(object) as *const Self) } } pub(crate) fn html_serialize( diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 864410f9292..29bb91a0d80 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -3201,7 +3201,7 @@ pub(crate) struct LayoutValue { #[allow(unsafe_code)] unsafe impl JSTraceable for LayoutValue { unsafe fn trace(&self, trc: *mut js::jsapi::JSTracer) { - self.value.trace(trc) + unsafe { self.value.trace(trc) }; } } diff --git a/components/script/layout_dom/element.rs b/components/script/layout_dom/element.rs index 4f8e6df9680..0fd806ab388 100644 --- a/components/script/layout_dom/element.rs +++ b/components/script/layout_dom/element.rs @@ -99,9 +99,11 @@ impl<'dom> ServoLayoutElement<'dom> { /// This function accesses and modifies the underlying DOM object and should /// not be used by more than a single thread at once. pub unsafe fn unset_snapshot_flags(&self) { - self.as_node() - .node - .set_flag(NodeFlags::HAS_SNAPSHOT | NodeFlags::HANDLED_SNAPSHOT, false); + unsafe { + self.as_node() + .node + .set_flag(NodeFlags::HAS_SNAPSHOT | NodeFlags::HANDLED_SNAPSHOT, false); + } } /// Unset the snapshot flags on the underlying DOM object for this element. @@ -111,7 +113,9 @@ impl<'dom> ServoLayoutElement<'dom> { /// This function accesses and modifies the underlying DOM object and should /// not be used by more than a single thread at once. pub unsafe fn set_has_snapshot(&self) { - self.as_node().node.set_flag(NodeFlags::HAS_SNAPSHOT, true); + unsafe { + self.as_node().node.set_flag(NodeFlags::HAS_SNAPSHOT, true); + } } /// Returns true if this element is the body child of an html element root element. @@ -355,22 +359,28 @@ impl<'dom> style::dom::TElement for ServoLayoutElement<'dom> { } unsafe fn set_handled_snapshot(&self) { - self.as_node() - .node - .set_flag(NodeFlags::HANDLED_SNAPSHOT, true); + unsafe { + self.as_node() + .node + .set_flag(NodeFlags::HANDLED_SNAPSHOT, true); + } } unsafe fn set_dirty_descendants(&self) { debug_assert!(self.as_node().is_connected()); - self.as_node() - .node - .set_flag(NodeFlags::HAS_DIRTY_DESCENDANTS, true) + unsafe { + self.as_node() + .node + .set_flag(NodeFlags::HAS_DIRTY_DESCENDANTS, true) + } } unsafe fn unset_dirty_descendants(&self) { - self.as_node() - .node - .set_flag(NodeFlags::HAS_DIRTY_DESCENDANTS, false) + unsafe { + self.as_node() + .node + .set_flag(NodeFlags::HAS_DIRTY_DESCENDANTS, false) + } } /// Whether this element should match user and content rules. @@ -406,11 +416,13 @@ impl<'dom> style::dom::TElement for ServoLayoutElement<'dom> { } unsafe fn clear_data(&self) { - self.as_node().get_jsmanaged().clear_style_and_layout_data() + unsafe { self.as_node().get_jsmanaged().clear_style_and_layout_data() } } unsafe fn ensure_data(&self) -> AtomicRefMut { - self.as_node().get_jsmanaged().initialize_style_data(); + unsafe { + self.as_node().get_jsmanaged().initialize_style_data(); + }; self.mutate_data().unwrap() } diff --git a/components/script/layout_dom/node.rs b/components/script/layout_dom/node.rs index 1ab47737dfd..516b541f864 100644 --- a/components/script/layout_dom/node.rs +++ b/components/script/layout_dom/node.rs @@ -77,7 +77,8 @@ impl<'dom> ServoLayoutNode<'dom> { /// /// The address pointed to by `address` should point to a valid node in memory. pub unsafe fn new(address: &TrustedNodeAddress) -> Self { - ServoLayoutNode::from_layout_js(LayoutDom::from_trusted_node_address(*address)) + let node = unsafe { LayoutDom::from_trusted_node_address(*address) }; + ServoLayoutNode::from_layout_js(node) } pub(super) fn script_type_id(&self) -> NodeTypeId { @@ -194,10 +195,10 @@ impl<'dom> LayoutNode<'dom> for ServoLayoutNode<'dom> { unsafe fn initialize_style_and_layout_data(&self) { let inner = self.get_jsmanaged(); if inner.style_data().is_none() { - inner.initialize_style_data(); + unsafe { inner.initialize_style_data() }; } if inner.layout_data().is_none() { - inner.initialize_layout_data(Box::::default()); + unsafe { inner.initialize_layout_data(Box::::default()) }; } } @@ -251,7 +252,8 @@ impl<'dom> ServoThreadSafeLayoutNode<'dom> { /// Get the first child of this node. Important: this is not safe for /// layout to call, so it should *never* be made public. unsafe fn dangerous_first_child(&self) -> Option { - self.get_jsmanaged() + let js_managed = unsafe { self.get_jsmanaged() }; + js_managed .first_child_ref() .map(ServoLayoutNode::from_layout_js) .map(Self::new) @@ -260,7 +262,8 @@ impl<'dom> ServoThreadSafeLayoutNode<'dom> { /// Get the next sibling of this node. Important: this is not safe for /// layout to call, so it should *never* be made public. unsafe fn dangerous_next_sibling(&self) -> Option { - self.get_jsmanaged() + let js_managed = unsafe { self.get_jsmanaged() }; + js_managed .next_sibling_ref() .map(ServoLayoutNode::from_layout_js) .map(Self::new) diff --git a/components/script/layout_dom/shadow_root.rs b/components/script/layout_dom/shadow_root.rs index 4c42db9d941..d648083f392 100644 --- a/components/script/layout_dom/shadow_root.rs +++ b/components/script/layout_dom/shadow_root.rs @@ -59,7 +59,9 @@ impl<'dom> ServoShadowRoot<'dom> { stylist: &mut Stylist, guard: &StyleSharedRwLockReadGuard, ) { - self.shadow_root - .flush_stylesheets::(stylist, guard) + unsafe { + self.shadow_root + .flush_stylesheets::(stylist, guard) + } } } diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index dae86b924af..809ded115de 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -179,7 +179,7 @@ pub(crate) fn with_script_thread(f: impl FnOnce(&ScriptThread) -> R) pub(crate) unsafe fn trace_thread(tr: *mut JSTracer) { with_script_thread(|script_thread| { trace!("tracing fields of ScriptThread"); - script_thread.trace(tr); + unsafe { script_thread.trace(tr) }; }) }