Auto merge of #22658 - servo:next-layout, r=SimonSapin

Make a bunch of layout queries morally safer

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22658)
<!-- Reviewable:end -->

components/layout/construct.rs

@@ -51,7 +51,7 @@ use crate::ServoArc;
 use script_layout_interface::wrapper_traits::{
     PseudoElementType, ThreadSafeLayoutElement, ThreadSafeLayoutNode,
 };
-use script_layout_interface::{is_image_data, LayoutElementType, LayoutNodeType};
+use script_layout_interface::{LayoutElementType, LayoutNodeType};
 use servo_config::opts;
 use servo_url::ServoUrl;
 use std::collections::LinkedList;
@@ -2472,3 +2472,9 @@ impl Legalizer {
         FlowRef::new(Arc::new(constructor(fragment)))
     }
 }
+
+pub fn is_image_data(uri: &str) -> bool {
+    static TYPES: &'static [&'static str] =
+        &["data:image/png", "data:image/gif", "data:image/jpeg"];
+    TYPES.iter().any(|&type_| uri.starts_with(type_))
+}

components/layout/query.rs

@@ -364,24 +364,24 @@ impl FragmentBorderBoxIterator for MarginRetrievingFragmentBorderBoxIterator {
     }
 }
 
-pub fn process_content_box_request<N: LayoutNode>(
-    requested_node: N,
+pub fn process_content_box_request(
+    requested_node: OpaqueNode,
     layout_root: &mut dyn Flow,
 ) -> Option<Rect<Au>> {
     // FIXME(pcwalton): This has not been updated to handle the stacking context relative
     // stuff. So the position is wrong in most cases.
-    let mut iterator = UnioningFragmentBorderBoxIterator::new(requested_node.opaque());
+    let mut iterator = UnioningFragmentBorderBoxIterator::new(requested_node);
     sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator);
     iterator.rect
 }
 
-pub fn process_content_boxes_request<N: LayoutNode>(
-    requested_node: N,
+pub fn process_content_boxes_request(
+    requested_node: OpaqueNode,
     layout_root: &mut dyn Flow,
 ) -> Vec<Rect<Au>> {
     // FIXME(pcwalton): This has not been updated to handle the stacking context relative
     // stuff. So the position is wrong in most cases.
-    let mut iterator = CollectingFragmentBorderBoxIterator::new(requested_node.opaque());
+    let mut iterator = CollectingFragmentBorderBoxIterator::new(requested_node);
     sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator);
     iterator.rects
 }
@@ -669,11 +669,11 @@ impl FragmentBorderBoxIterator for ParentOffsetBorderBoxIterator {
     }
 }
 
-pub fn process_node_geometry_request<N: LayoutNode>(
-    requested_node: N,
+pub fn process_node_geometry_request(
+    requested_node: OpaqueNode,
     layout_root: &mut dyn Flow,
 ) -> Rect<i32> {
-    let mut iterator = FragmentLocatingFragmentIterator::new(requested_node.opaque());
+    let mut iterator = FragmentLocatingFragmentIterator::new(requested_node);
     sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator);
     iterator.client_rect
 }
@@ -687,11 +687,11 @@ pub fn process_node_scroll_id_request<N: LayoutNode>(
 }
 
 /// https://drafts.csswg.org/cssom-view/#scrolling-area
-pub fn process_node_scroll_area_request<N: LayoutNode>(
-    requested_node: N,
+pub fn process_node_scroll_area_request(
+    requested_node: OpaqueNode,
     layout_root: &mut dyn Flow,
 ) -> Rect<i32> {
-    let mut iterator = UnioningFragmentScrollAreaIterator::new(requested_node.opaque());
+    let mut iterator = UnioningFragmentScrollAreaIterator::new(requested_node);
     sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator);
     match iterator.overflow_direction {
         OverflowDirection::RightAndDown => {
@@ -932,11 +932,11 @@ where
     }
 }
 
-pub fn process_offset_parent_query<N: LayoutNode>(
-    requested_node: N,
+pub fn process_offset_parent_query(
+    requested_node: OpaqueNode,
     layout_root: &mut dyn Flow,
 ) -> OffsetParentResponse {
-    let mut iterator = ParentOffsetBorderBoxIterator::new(requested_node.opaque());
+    let mut iterator = ParentOffsetBorderBoxIterator::new(requested_node);
     sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator);
 
     let node_offset_box = iterator.node_offset_box;

components/layout_thread/lib.rs

@@ -1471,32 +1471,23 @@ impl LayoutThread {
         match *reflow_goal {
             ReflowGoal::LayoutQuery(ref querymsg, _) => match querymsg {
                 &QueryMsg::ContentBoxQuery(node) => {
-                    let node = unsafe { ServoLayoutNode::new(&node) };
                     rw_data.content_box_response = process_content_box_request(node, root_flow);
                 },
                 &QueryMsg::ContentBoxesQuery(node) => {
-                    let node = unsafe { ServoLayoutNode::new(&node) };
                     rw_data.content_boxes_response = process_content_boxes_request(node, root_flow);
                 },
                 &QueryMsg::TextIndexQuery(node, point_in_node) => {
-                    let node = unsafe { ServoLayoutNode::new(&node) };
-                    let opaque_node = node.opaque();
                     let point_in_node = Point2D::new(
                         Au::from_f32_px(point_in_node.x),
                         Au::from_f32_px(point_in_node.y),
                     );
-                    rw_data.text_index_response = TextIndexResponse(
-                        rw_data
-                            .indexable_text
-                            .text_index(opaque_node, point_in_node),
-                    );
+                    rw_data.text_index_response =
+                        TextIndexResponse(rw_data.indexable_text.text_index(node, point_in_node));
                 },
                 &QueryMsg::NodeGeometryQuery(node) => {
-                    let node = unsafe { ServoLayoutNode::new(&node) };
                     rw_data.client_rect_response = process_node_geometry_request(node, root_flow);
                 },
                 &QueryMsg::NodeScrollGeometryQuery(node) => {
-                    let node = unsafe { ServoLayoutNode::new(&node) };
                     rw_data.scroll_area_response =
                         process_node_scroll_area_request(node, root_flow);
                 },
@@ -1511,7 +1502,6 @@ impl LayoutThread {
                         process_resolved_style_request(context, node, pseudo, property, root_flow);
                 },
                 &QueryMsg::OffsetParentQuery(node) => {
-                    let node = unsafe { ServoLayoutNode::new(&node) };
                     rw_data.offset_parent_response = process_offset_parent_query(node, root_flow);
                 },
                 &QueryMsg::StyleQuery(node) => {

components/script/dom/htmlelement.rs

@@ -403,7 +403,7 @@ impl HTMLElementMethods for HTMLElement {
 
         let node = self.upcast::<Node>();
         let window = window_from_node(self);
-        let (element, _) = window.offset_parent_query(node.to_trusted_node_address());
+        let (element, _) = window.offset_parent_query(node);
 
         element
     }
@@ -416,7 +416,7 @@ impl HTMLElementMethods for HTMLElement {
 
         let node = self.upcast::<Node>();
         let window = window_from_node(self);
-        let (_, rect) = window.offset_parent_query(node.to_trusted_node_address());
+        let (_, rect) = window.offset_parent_query(node);
 
         rect.origin.y.to_nearest_px()
     }
@@ -429,7 +429,7 @@ impl HTMLElementMethods for HTMLElement {
 
         let node = self.upcast::<Node>();
         let window = window_from_node(self);
-        let (_, rect) = window.offset_parent_query(node.to_trusted_node_address());
+        let (_, rect) = window.offset_parent_query(node);
 
         rect.origin.x.to_nearest_px()
     }
@@ -438,7 +438,7 @@ impl HTMLElementMethods for HTMLElement {
     fn OffsetWidth(&self) -> i32 {
         let node = self.upcast::<Node>();
         let window = window_from_node(self);
-        let (_, rect) = window.offset_parent_query(node.to_trusted_node_address());
+        let (_, rect) = window.offset_parent_query(node);
 
         rect.size.width.to_nearest_px()
     }
@@ -447,7 +447,7 @@ impl HTMLElementMethods for HTMLElement {
     fn OffsetHeight(&self) -> i32 {
         let node = self.upcast::<Node>();
         let window = window_from_node(self);
-        let (_, rect) = window.offset_parent_query(node.to_trusted_node_address());
+        let (_, rect) = window.offset_parent_query(node);
 
         rect.size.height.to_nearest_px()
     }

components/script/dom/htmlinputelement.rs

@@ -1467,10 +1467,8 @@ impl VirtualMethods for HTMLInputElement {
                     // now.
                     if let Some(point_in_target) = mouse_event.point_in_target() {
                         let window = window_from_node(self);
-                        let TextIndexResponse(index) = window.text_index_query(
-                            self.upcast::<Node>().to_trusted_node_address(),
-                            point_in_target,
-                        );
+                        let TextIndexResponse(index) =
+                            window.text_index_query(self.upcast::<Node>(), point_in_target);
                         if let Some(i) = index {
                             self.textinput.borrow_mut().set_edit_point_index(i as usize);
                             // trigger redraw

components/script/dom/node.rs

@@ -334,6 +334,10 @@ impl Node {
         UntrustedNodeAddress(self.reflector().get_jsobject().get() as *const c_void)
     }
 
+    pub fn to_opaque(&self) -> OpaqueNode {
+        OpaqueNode(self.reflector().get_jsobject().get() as usize)
+    }
+
     pub fn as_custom_element(&self) -> Option<DomRoot<Element>> {
         self.downcast::<Element>().and_then(|element| {
             if element.get_custom_element_definition().is_some() {
@@ -639,19 +643,19 @@ impl Node {
     /// Returns the rendered bounding content box if the element is rendered,
     /// and none otherwise.
     pub fn bounding_content_box(&self) -> Option<Rect<Au>> {
-        window_from_node(self).content_box_query(self.to_trusted_node_address())
+        window_from_node(self).content_box_query(self)
     }
 
     pub fn bounding_content_box_or_zero(&self) -> Rect<Au> {
         self.bounding_content_box().unwrap_or_else(Rect::zero)
     }
 
     pub fn content_boxes(&self) -> Vec<Rect<Au>> {
-        window_from_node(self).content_boxes_query(self.to_trusted_node_address())
+        window_from_node(self).content_boxes_query(self)
     }
 
     pub fn client_rect(&self) -> Rect<i32> {
-        window_from_node(self).client_rect_query(self.to_trusted_node_address())
+        window_from_node(self).client_rect_query(self)
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-element-scrollwidth
@@ -668,7 +672,7 @@ impl Node {
             .downcast::<HTMLBodyElement>()
             .map_or(false, |e| e.is_the_html_body_element());
 
-        let scroll_area = window.scroll_area_query(self.to_trusted_node_address());
+        let scroll_area = window.scroll_area_query(self);
 
         match (
             document != window.Document(),

components/script/dom/window.rs

@@ -1571,31 +1571,31 @@ impl Window {
         &*self.layout_rpc
     }
 
-    pub fn content_box_query(&self, content_box_request: TrustedNodeAddress) -> Option<Rect<Au>> {
-        if !self.layout_reflow(QueryMsg::ContentBoxQuery(content_box_request)) {
+    pub fn content_box_query(&self, node: &Node) -> Option<Rect<Au>> {
+        if !self.layout_reflow(QueryMsg::ContentBoxQuery(node.to_opaque())) {
             return None;
         }
         let ContentBoxResponse(rect) = self.layout_rpc.content_box();
         rect
     }
 
-    pub fn content_boxes_query(&self, content_boxes_request: TrustedNodeAddress) -> Vec<Rect<Au>> {
-        if !self.layout_reflow(QueryMsg::ContentBoxesQuery(content_boxes_request)) {
+    pub fn content_boxes_query(&self, node: &Node) -> Vec<Rect<Au>> {
+        if !self.layout_reflow(QueryMsg::ContentBoxesQuery(node.to_opaque())) {
             return vec![];
         }
         let ContentBoxesResponse(rects) = self.layout_rpc.content_boxes();
         rects
     }
 
-    pub fn client_rect_query(&self, node_geometry_request: TrustedNodeAddress) -> Rect<i32> {
-        if !self.layout_reflow(QueryMsg::NodeGeometryQuery(node_geometry_request)) {
+    pub fn client_rect_query(&self, node: &Node) -> Rect<i32> {
+        if !self.layout_reflow(QueryMsg::NodeGeometryQuery(node.to_opaque())) {
             return Rect::zero();
         }
         self.layout_rpc.node_geometry().client_rect
     }
 
-    pub fn scroll_area_query(&self, node: TrustedNodeAddress) -> Rect<i32> {
-        if !self.layout_reflow(QueryMsg::NodeScrollGeometryQuery(node)) {
+    pub fn scroll_area_query(&self, node: &Node) -> Rect<i32> {
+        if !self.layout_reflow(QueryMsg::NodeScrollGeometryQuery(node.to_opaque())) {
             return Rect::zero();
         }
         self.layout_rpc.node_scroll_area().client_rect
@@ -1652,14 +1652,13 @@ impl Window {
     }
 
     #[allow(unsafe_code)]
-    pub fn offset_parent_query(
-        &self,
-        node: TrustedNodeAddress,
-    ) -> (Option<DomRoot<Element>>, Rect<Au>) {
-        if !self.layout_reflow(QueryMsg::OffsetParentQuery(node)) {
+    pub fn offset_parent_query(&self, node: &Node) -> (Option<DomRoot<Element>>, Rect<Au>) {
+        if !self.layout_reflow(QueryMsg::OffsetParentQuery(node.to_opaque())) {
             return (None, Rect::zero());
         }
 
+        // FIXME(nox): Layout can reply with a garbage value which doesn't
+        // actually correspond to an element, that's unsound.
         let response = self.layout_rpc.offset_parent();
         let js_runtime = self.js_runtime.borrow();
         let js_runtime = js_runtime.as_ref().unwrap();
@@ -1677,12 +1676,8 @@ impl Window {
         self.layout_rpc.style().0
     }
 
-    pub fn text_index_query(
-        &self,
-        node: TrustedNodeAddress,
-        point_in_node: Point2D<f32>,
-    ) -> TextIndexResponse {
-        if !self.layout_reflow(QueryMsg::TextIndexQuery(node, point_in_node)) {
+    pub fn text_index_query(&self, node: &Node, point_in_node: Point2D<f32>) -> TextIndexResponse {
+        if !self.layout_reflow(QueryMsg::TextIndexQuery(node.to_opaque(), point_in_node)) {
             return TextIndexResponse(None);
         }
         self.layout_rpc.text_index()

components/script_layout_interface/lib.rs

@@ -7,7 +7,6 @@
 //! to depend on script.
 
 #![deny(unsafe_code)]
-#![feature(associated_type_defaults)]
 
 #[macro_use]
 extern crate html5ever;
@@ -129,12 +128,6 @@ pub struct TrustedNodeAddress(pub *const c_void);
 #[allow(unsafe_code)]
 unsafe impl Send for TrustedNodeAddress {}
 
-pub fn is_image_data(uri: &str) -> bool {
-    static TYPES: &'static [&'static str] =
-        &["data:image/png", "data:image/gif", "data:image/jpeg"];
-    TYPES.iter().any(|&type_| uri.starts_with(type_))
-}
-
 /// Whether the pending image needs to be fetched or is waiting on an existing fetch.
 pub enum PendingImageState {
     Unrequested(ServoUrl),

components/script_layout_interface/message.rs

@@ -21,6 +21,7 @@ use servo_atoms::Atom;
 use servo_url::ServoUrl;
 use std::sync::Arc;
 use style::context::QuirksMode;
+use style::dom::OpaqueNode;
 use style::properties::PropertyId;
 use style::selector_parser::PseudoElement;
 use style::stylesheets::Stylesheet;
@@ -111,16 +112,20 @@ pub enum NodesFromPointQueryType {
 
 #[derive(Debug, PartialEq)]
 pub enum QueryMsg {
-    ContentBoxQuery(TrustedNodeAddress),
-    ContentBoxesQuery(TrustedNodeAddress),
+    ContentBoxQuery(OpaqueNode),
+    ContentBoxesQuery(OpaqueNode),
+    NodeGeometryQuery(OpaqueNode),
+    NodeScrollGeometryQuery(OpaqueNode),
+    OffsetParentQuery(OpaqueNode),
+    TextIndexQuery(OpaqueNode, Point2D<f32>),
+    NodesFromPointQuery(Point2D<f32>, NodesFromPointQueryType),
+
+    // FIXME(nox): The following queries use the TrustedNodeAddress to
+    // access actual DOM nodes, but those values can be constructed from
+    // garbage values such as `0xdeadbeef as *const _`, this is unsound.
     NodeScrollIdQuery(TrustedNodeAddress),
-    NodeGeometryQuery(TrustedNodeAddress),
-    NodeScrollGeometryQuery(TrustedNodeAddress),
     ResolvedStyleQuery(TrustedNodeAddress, Option<PseudoElement>, PropertyId),
-    OffsetParentQuery(TrustedNodeAddress),
     StyleQuery(TrustedNodeAddress),
-    TextIndexQuery(TrustedNodeAddress, Point2D<f32>),
-    NodesFromPointQuery(Point2D<f32>, NodesFromPointQueryType),
     ElementInnerTextQuery(TrustedNodeAddress),
 }