script: Reduce the use of unsafe in LayoutDom (#31979)

Remove the use of unsafe code in the layout wrappers of the DOM. The
main change here is that `unsafe_get()` no longer needs to be an unsafe
method, which allows us to transitively remove or reduce unsafe blocks
from callers. The function itself is not renamed, because it's still
a bit dangerous to start removing the layers of abstraction from actual
DOM nodes.

In addition `init_style_and_opaque_layout_data` can be merged into
`initialize_data`, which removes one more unsafe method.

Finally, a "Safety" section is added to some unsafe methods.

components/layout/wrapper.rs

@@ -140,7 +140,7 @@ where
         // We need the underlying node to potentially access the parent in the
         // case of text nodes. This is safe as long as we don't let the parent
         // escape and never access its descendants.
-        let mut node = unsafe { self.unsafe_get() };
+        let mut node = self.unsafe_get();
 
         // If this is a text node, use the parent element, since that's what
         // controls our style.

components/script/dom/attr.rs

@@ -268,11 +268,11 @@ impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> {
 
     #[inline]
     fn local_name(self) -> &'dom LocalName {
-        unsafe { &self.unsafe_get().identifier.local_name.0 }
+        &self.unsafe_get().identifier.local_name.0
     }
 
     #[inline]
     fn namespace(self) -> &'dom Namespace {
-        unsafe { &self.unsafe_get().identifier.namespace.0 }
+        &self.unsafe_get().identifier.namespace.0
     }
 }

components/script/dom/bindings/root.rs

@@ -757,7 +757,7 @@ where
 {
     /// Returns a reference to the interior of this JS object. The fact
     /// that this is unsafe is what necessitates the layout wrappers.
-    pub unsafe fn unsafe_get(self) -> &'dom T {
+    pub fn unsafe_get(self) -> &'dom T {
         assert_in_layout();
         self.value
     }

components/script/dom/canvasrenderingcontext2d.rs

@@ -127,23 +127,20 @@ impl CanvasRenderingContext2D {
 }
 
 pub trait LayoutCanvasRenderingContext2DHelpers {
-    #[allow(unsafe_code)]
+    fn get_ipc_renderer(self) -> IpcSender<CanvasMsg>;
-    unsafe fn get_ipc_renderer(self) -> IpcSender<CanvasMsg>;
     fn get_canvas_id(self) -> CanvasId;
 }
 
 impl LayoutCanvasRenderingContext2DHelpers for LayoutDom<'_, CanvasRenderingContext2D> {
-    #[allow(unsafe_code)]
+    fn get_ipc_renderer(self) -> IpcSender<CanvasMsg> {
-    unsafe fn get_ipc_renderer(self) -> IpcSender<CanvasMsg> {
         (self.unsafe_get()).canvas_state.get_ipc_renderer().clone()
     }
 
-    #[allow(unsafe_code)]
     fn get_canvas_id(self) -> CanvasId {
         // FIXME(nox): This relies on the fact that CanvasState::get_canvas_id
         // does nothing fancy but it would be easier to trust a
         // LayoutDom<_>-like type that would wrap the &CanvasState.
-        unsafe { self.unsafe_get().canvas_state.get_canvas_id() }
+        self.unsafe_get().canvas_state.get_canvas_id()
     }
 }
 

components/script/dom/document.rs

@@ -2934,40 +2934,40 @@ pub enum DocumentSource {
 #[allow(unsafe_code)]
 pub trait LayoutDocumentHelpers<'dom> {
     fn is_html_document_for_layout(self) -> bool;
-    unsafe fn needs_paint_from_layout(self);
+    fn needs_paint_from_layout(self);
-    unsafe fn will_paint(self);
+    fn will_paint(self);
     fn quirks_mode(self) -> QuirksMode;
     fn style_shared_lock(self) -> &'dom StyleSharedRwLock;
     fn shadow_roots(self) -> Vec<LayoutDom<'dom, ShadowRoot>>;
     fn shadow_roots_styles_changed(self) -> bool;
-    unsafe fn flush_shadow_roots_stylesheets(self);
+    fn flush_shadow_roots_stylesheets(self);
 }
 
 #[allow(unsafe_code)]
 impl<'dom> LayoutDocumentHelpers<'dom> for LayoutDom<'dom, Document> {
     #[inline]
     fn is_html_document_for_layout(self) -> bool {
-        unsafe { self.unsafe_get().is_html_document }
+        self.unsafe_get().is_html_document
     }
 
     #[inline]
-    unsafe fn needs_paint_from_layout(self) {
+    fn needs_paint_from_layout(self) {
         (self.unsafe_get()).needs_paint.set(true)
     }
 
     #[inline]
-    unsafe fn will_paint(self) {
+    fn will_paint(self) {
         (self.unsafe_get()).needs_paint.set(false)
     }
 
     #[inline]
     fn quirks_mode(self) -> QuirksMode {
-        unsafe { self.unsafe_get().quirks_mode.get() }
+        self.unsafe_get().quirks_mode.get()
     }
 
     #[inline]
     fn style_shared_lock(self) -> &'dom StyleSharedRwLock {
-        unsafe { self.unsafe_get().style_shared_lock() }
+        self.unsafe_get().style_shared_lock()
     }
 
     #[inline]
@@ -2988,11 +2988,11 @@ impl<'dom> LayoutDocumentHelpers<'dom> for LayoutDom<'dom, Document> {
 
     #[inline]
     fn shadow_roots_styles_changed(self) -> bool {
-        unsafe { self.unsafe_get().shadow_roots_styles_changed.get() }
+        self.unsafe_get().shadow_roots_styles_changed.get()
     }
 
     #[inline]
-    unsafe fn flush_shadow_roots_stylesheets(self) {
+    fn flush_shadow_roots_stylesheets(self) {
         (*self.unsafe_get()).flush_shadow_roots_stylesheets()
     }
 }

components/script/dom/element.rs

@@ -641,9 +641,8 @@ pub trait LayoutElementHelpers<'dom> {
 }
 
 impl<'dom> LayoutDom<'dom, Element> {
-    #[allow(unsafe_code)]
     pub(super) fn focus_state(self) -> bool {
-        unsafe { self.unsafe_get().state.get().contains(ElementState::FOCUS) }
+        self.unsafe_get().state.get().contains(ElementState::FOCUS)
     }
 }
 
@@ -1059,12 +1058,11 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> {
 
     #[allow(unsafe_code)]
     fn local_name(self) -> &'dom LocalName {
-        unsafe { &(self.unsafe_get()).local_name }
+        &(self.unsafe_get()).local_name
     }
 
-    #[allow(unsafe_code)]
     fn namespace(self) -> &'dom Namespace {
-        unsafe { &(self.unsafe_get()).namespace }
+        &(self.unsafe_get()).namespace
     }
 
     fn get_lang_for_layout(self) -> String {
@@ -1091,25 +1089,20 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> {
     }
 
     #[inline]
-    #[allow(unsafe_code)]
     fn get_state_for_layout(self) -> ElementState {
-        unsafe { (self.unsafe_get()).state.get() }
+        (self.unsafe_get()).state.get()
     }
 
     #[inline]
-    #[allow(unsafe_code)]
     fn insert_selector_flags(self, flags: ElementSelectorFlags) {
         debug_assert!(thread_state::get().is_layout());
-        unsafe {
         let f = &(self.unsafe_get()).selector_flags;
         f.set(f.get() | flags);
     }
-    }
 
     #[inline]
-    #[allow(unsafe_code)]
     fn get_selector_flags(self) -> ElementSelectorFlags {
-        unsafe { self.unsafe_get().selector_flags.get() }
+        self.unsafe_get().selector_flags.get()
     }
 
     #[inline]

components/script/dom/gpucanvascontext.rs

@@ -199,8 +199,7 @@ impl GPUCanvasContext {
 }
 
 impl LayoutCanvasRenderingContextHelpers for LayoutDom<'_, GPUCanvasContext> {
-    #[allow(unsafe_code)]
+    fn canvas_data_source(self) -> HTMLCanvasDataSource {
-    unsafe fn canvas_data_source(self) -> HTMLCanvasDataSource {
         (*self.unsafe_get()).layout_handle()
     }
 }

components/script/dom/htmlcanvaselement.rs

@@ -122,8 +122,7 @@ impl HTMLCanvasElement {
 }
 
 pub trait LayoutCanvasRenderingContextHelpers {
-    #[allow(unsafe_code)]
+    fn canvas_data_source(self) -> HTMLCanvasDataSource;
-    unsafe fn canvas_data_source(self) -> HTMLCanvasDataSource;
 }
 
 pub trait LayoutHTMLCanvasElementHelpers {
@@ -178,8 +177,8 @@ impl LayoutHTMLCanvasElementHelpers for LayoutDom<'_, HTMLCanvasElement> {
 
     #[allow(unsafe_code)]
     fn get_canvas_id_for_layout(self) -> CanvasId {
-        unsafe {
         let canvas = self.unsafe_get();
+        unsafe {
             if let &Some(CanvasContext::Context2d(ref context)) = canvas.context.borrow_for_layout()
             {
                 context.to_layout().get_canvas_id()

components/script/dom/htmliframeelement.rs

@@ -515,15 +515,13 @@ pub trait HTMLIFrameElementLayoutMethods {
 
 impl HTMLIFrameElementLayoutMethods for LayoutDom<'_, HTMLIFrameElement> {
     #[inline]
-    #[allow(unsafe_code)]
     fn pipeline_id(self) -> Option<PipelineId> {
-        unsafe { (self.unsafe_get()).pipeline_id.get() }
+        (self.unsafe_get()).pipeline_id.get()
     }
 
     #[inline]
-    #[allow(unsafe_code)]
     fn browsing_context_id(self) -> Option<BrowsingContextId> {
-        unsafe { (self.unsafe_get()).browsing_context_id.get() }
+        (self.unsafe_get()).browsing_context_id.get()
     }
 
     fn get_width(self) -> LengthOrPercentageOrAuto {

components/script/dom/htmlinputelement.rs

@@ -1000,7 +1000,7 @@ impl<'dom> LayoutDom<'dom, HTMLInputElement> {
     }
 
     fn input_type(self) -> InputType {
-        unsafe { self.unsafe_get().input_type.get() }
+        self.unsafe_get().input_type.get()
     }
 
     fn textinput_sorted_selection_offsets_range(self) -> Range<UTF8Bytes> {
@@ -1054,9 +1054,8 @@ impl<'dom> LayoutHTMLInputElementHelpers<'dom> for LayoutDom<'dom, HTMLInputElem
         }
     }
 
-    #[allow(unsafe_code)]
     fn size_for_layout(self) -> u32 {
-        unsafe { self.unsafe_get().size.get() }
+        self.unsafe_get().size.get()
     }
 
     fn selection_for_layout(self) -> Option<Range<usize>> {

components/script/dom/htmlmediaelement.rs

@@ -2482,11 +2482,14 @@ pub trait LayoutHTMLMediaElementHelpers {
 }
 
 impl LayoutHTMLMediaElementHelpers for LayoutDom<'_, HTMLMediaElement> {
-    #[allow(unsafe_code)]
     fn data(self) -> HTMLMediaData {
-        let media = unsafe { self.unsafe_get() };
         HTMLMediaData {
-            current_frame: media.video_renderer.lock().unwrap().current_frame,
+            current_frame: self
+                .unsafe_get()
+                .video_renderer
+                .lock()
+                .unwrap()
+                .current_frame,
         }
     }
 }

components/script/dom/htmltableelement.rs

@@ -440,19 +440,16 @@ impl HTMLTableElementLayoutHelpers for LayoutDom<'_, HTMLTableElement> {
             .cloned()
     }
 
-    #[allow(unsafe_code)]
     fn get_border(self) -> Option<u32> {
-        unsafe { (self.unsafe_get()).border.get() }
+        (self.unsafe_get()).border.get()
     }
 
-    #[allow(unsafe_code)]
     fn get_cellpadding(self) -> Option<u32> {
-        unsafe { (self.unsafe_get()).cellpadding.get() }
+        (self.unsafe_get()).cellpadding.get()
     }
 
-    #[allow(unsafe_code)]
     fn get_cellspacing(self) -> Option<u32> {
-        unsafe { (self.unsafe_get()).cellspacing.get() }
+        (self.unsafe_get()).cellspacing.get()
     }
 
     fn get_width(self) -> LengthOrPercentageOrAuto {

components/script/dom/node.rs

@@ -1344,7 +1344,23 @@ pub trait LayoutNodeHelpers<'dom> {
     fn children_count(self) -> u32;
 
     fn get_style_and_opaque_layout_data(self) -> Option<&'dom StyleAndOpaqueLayoutData>;
+
+    /// Initialize the style and opaque layout data of this node.
+    ///
+    /// # Safety
+    ///
+    /// This method is unsafe because it modifies the given node during
+    /// layout. Callers should ensure that no other layout thread is
+    /// attempting to read or modify the opaque layout data of this node.
     unsafe fn init_style_and_opaque_layout_data(self, data: Box<StyleAndOpaqueLayoutData>);
+
+    /// Unset and return the style and opaque layout data of this node.
+    ///
+    /// # Safety
+    ///
+    /// This method is unsafe because it modifies the given node during
+    /// layout. Callers should ensure that no other layout thread is
+    /// attempting to read or modify the opaque layout data of this node.
     unsafe fn take_style_and_opaque_layout_data(self) -> Box<StyleAndOpaqueLayoutData>;
 
     fn text_content(self) -> Cow<'dom, str>;
@@ -1370,9 +1386,8 @@ impl<'dom> LayoutDom<'dom, Node> {
 
 impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> {
     #[inline]
-    #[allow(unsafe_code)]
     fn type_id_for_layout(self) -> NodeTypeId {
-        unsafe { self.unsafe_get().type_id() }
+        self.unsafe_get().type_id()
     }
 
     #[inline]
@@ -1461,9 +1476,8 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> {
     }
 
     #[inline]
-    #[allow(unsafe_code)]
     fn children_count(self) -> u32 {
-        unsafe { self.unsafe_get().children_count.get() }
+        self.unsafe_get().children_count.get()
     }
 
     // FIXME(nox): How we handle style and layout data needs to be completely

components/script/dom/webgl2renderingcontext.rs

@@ -4474,8 +4474,8 @@ impl WebGL2RenderingContextMethods for WebGL2RenderingContext {
 
 impl LayoutCanvasRenderingContextHelpers for LayoutDom<'_, WebGL2RenderingContext> {
     #[allow(unsafe_code)]
-    unsafe fn canvas_data_source(self) -> HTMLCanvasDataSource {
+    fn canvas_data_source(self) -> HTMLCanvasDataSource {
         let this = self.unsafe_get();
-        (*this.base.to_layout().unsafe_get()).layout_handle()
+        unsafe { (*this.base.to_layout().unsafe_get()).layout_handle() }
     }
 }

components/script/dom/webglrenderingcontext.rs

@@ -4680,8 +4680,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 }
 
 impl LayoutCanvasRenderingContextHelpers for LayoutDom<'_, WebGLRenderingContext> {
-    #[allow(unsafe_code)]
+    fn canvas_data_source(self) -> HTMLCanvasDataSource {
-    unsafe fn canvas_data_source(self) -> HTMLCanvasDataSource {
         (*self.unsafe_get()).layout_handle()
     }
 }

components/script/layout_dom/document.rs

@@ -64,11 +64,11 @@ impl<'ld, LayoutDataType: LayoutDataTrait> ServoLayoutDocument<'ld, LayoutDataTy
     }
 
     pub fn needs_paint_from_layout(&self) {
-        unsafe { self.document.needs_paint_from_layout() }
+        self.document.needs_paint_from_layout()
     }
 
     pub fn will_paint(&self) {
-        unsafe { self.document.will_paint() }
+        self.document.will_paint()
     }
 
     pub fn style_shared_lock(&self) -> &StyleSharedRwLock {

components/script/layout_dom/element.rs

@@ -323,7 +323,11 @@ impl<'dom, LayoutDataType: LayoutDataTrait> style::dom::TElement
 
     unsafe fn clear_data(&self) {
         if self.get_style_and_opaque_layout_data().is_some() {
-            drop(self.as_node().take_style_and_opaque_layout_data());
+            drop(
+                self.as_node()
+                    .get_jsmanaged()
+                    .take_style_and_opaque_layout_data(),
+            );
         }
     }
 
@@ -758,7 +762,7 @@ impl<'dom, LayoutDataType: LayoutDataTrait> ThreadSafeLayoutElement<'dom>
         self.as_node().type_id()
     }
 
-    unsafe fn unsafe_get(self) -> ServoLayoutElement<'dom, LayoutDataType> {
+    fn unsafe_get(self) -> ServoLayoutElement<'dom, LayoutDataType> {
         self.element
     }
 

components/script/layout_dom/node.rs

@@ -207,21 +207,17 @@ impl<'dom, LayoutDataType: LayoutDataTrait> LayoutNode<'dom>
     }
 
     unsafe fn initialize_data(&self) {
-        if self.get_style_and_opaque_layout_data().is_none() {
+        if self.get_style_and_opaque_layout_data().is_some() {
+            return;
+        }
+
         let opaque = StyleAndOpaqueLayoutData::new(
             StyleData::default(),
             AtomicRefCell::new(LayoutDataType::default()),
         );
-            self.init_style_and_opaque_layout_data(opaque);
-        };
-    }
 
-    unsafe fn init_style_and_opaque_layout_data(&self, data: Box<StyleAndOpaqueLayoutData>) {
+        self.get_jsmanaged()
-        self.get_jsmanaged().init_style_and_opaque_layout_data(data);
+            .init_style_and_opaque_layout_data(opaque);
-    }
-
-    unsafe fn take_style_and_opaque_layout_data(&self) -> Box<StyleAndOpaqueLayoutData> {
-        self.get_jsmanaged().take_style_and_opaque_layout_data()
     }
 
     fn is_connected(&self) -> bool {
@@ -402,7 +398,7 @@ impl<'dom, LayoutDataType: LayoutDataTrait> ThreadSafeLayoutNode<'dom>
         }
     }
 
-    unsafe fn unsafe_get(self) -> Self::ConcreteNode {
+    fn unsafe_get(self) -> Self::ConcreteNode {
         self.node
     }
 

components/shared/script_layout/wrapper_traits.rs

@@ -89,9 +89,14 @@ pub trait LayoutNode<'dom>:
     /// Returns the type ID of this node.
     fn type_id(&self) -> LayoutNodeType;
 
+    /// Initialize this node with empty style and opaque layout data.
+    ///
+    /// # Safety
+    ///
+    /// This method is unsafe because it modifies the given node during
+    /// layout. Callers should ensure that no other layout thread is
+    /// attempting to read or modify the opaque layout data of this node.
     unsafe fn initialize_data(&self);
-    unsafe fn init_style_and_opaque_layout_data(&self, data: Box<StyleAndOpaqueLayoutData>);
-    unsafe fn take_style_and_opaque_layout_data(&self) -> Box<StyleAndOpaqueLayoutData>;
 
     fn rev_children(self) -> LayoutIterator<ReverseChildrenIterator<Self>> {
         LayoutIterator(ReverseChildrenIterator {
@@ -259,7 +264,7 @@ pub trait ThreadSafeLayoutNode<'dom>:
     ///
     /// We need this because the implementation of some methods need to access the layout
     /// data flags, and we have this annoying trait separation between script and layout :-(
-    unsafe fn unsafe_get(self) -> Self::ConcreteNode;
+    fn unsafe_get(self) -> Self::ConcreteNode;
 
     fn node_text_content(self) -> Cow<'dom, str>;
 
@@ -338,7 +343,7 @@ pub trait ThreadSafeLayoutElement<'dom>:
     ///
     /// We need this so that the functions defined on this trait can call
     /// lazily_compute_pseudo_element_style, which operates on TElement.
-    unsafe fn unsafe_get(self) -> Self::ConcreteElement;
+    fn unsafe_get(self) -> Self::ConcreteElement;
 
     /// Get the local name of this element. See
     /// <https://dom.spec.whatwg.org/#concept-element-local-name>.
@@ -437,7 +442,7 @@ pub trait ThreadSafeLayoutElement<'dom>:
                             .stylist
                             .lazily_compute_pseudo_element_style(
                                 &context.guards,
-                                unsafe { self.unsafe_get() },
+                                self.unsafe_get(),
                                 &style_pseudo,
                                 RuleInclusion::All,
                                 data.styles.primary(),