From 5a5d796988789ce6fe0bc3f0d72bcb80506208ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20W=C3=BClker?= Date: Fri, 7 Feb 2025 02:05:27 +0100 Subject: [PATCH] Implement ServoLayoutNode::traversal_parent (#35338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes common crash related to slottables, currently present on wpt.fyi. Previously, the traversal parent of `Text` nodes was incorrectly assumed to always be the parent or shadow host. That caused crashes inside stylo's bloom filter. Now the traversal parent is the slot that the node is assigned to, if any, and the parent/shadow host otherwise. The slottable data for Text/Element nodes is now stored in NodeRareData. This is very cheap, because NodeRareData will already be instantiated for assigned slottables anyways, because the containing_shadow_root field will be set (since assigned slottables are always in a shadow tree). This change is necessary because we need to hand out references to the assigned slot to stylo and that is not possible to do (without unsafe code) if we need to downcast the node first. As a side effect, this reduces the size of `Text` from 256 to 232 bytes, because the slottable data is no longer stored there. Signed-off-by: Simon Wülker --- components/script/dom/element.rs | 52 ------------------ components/script/dom/eventtarget.rs | 22 +------- components/script/dom/htmlslotelement.rs | 61 ++------------------- components/script/dom/node.rs | 67 +++++++++++++++++++----- components/script/dom/raredata.rs | 6 +-- components/script/dom/text.rs | 10 +--- components/script/layout_dom/element.rs | 13 +---- components/script/layout_dom/node.rs | 11 ++++ tests/unit/script/size_of.rs | 2 +- 9 files changed, 77 insertions(+), 167 deletions(-) diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 24e760d510c..66e977bf2b5 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -613,43 +613,6 @@ impl Element { Some(node) => node.is::(), } } - - pub(crate) fn assigned_slot(&self) -> Option> { - let assigned_slot = self - .rare_data - .borrow() - .as_ref()? - .slottable_data - .assigned_slot - .as_ref()? - .as_rooted(); - Some(assigned_slot) - } - - pub(crate) fn set_assigned_slot(&self, assigned_slot: Option<&HTMLSlotElement>) { - self.ensure_rare_data().slottable_data.assigned_slot = assigned_slot.map(Dom::from_ref); - } - - pub(crate) fn manual_slot_assignment(&self) -> Option> { - let manually_assigned_slot = self - .rare_data - .borrow() - .as_ref()? - .slottable_data - .manual_slot_assignment - .as_ref()? - .as_rooted(); - Some(manually_assigned_slot) - } - - pub(crate) fn set_manual_slot_assignment( - &self, - manually_assigned_slot: Option<&HTMLSlotElement>, - ) { - self.ensure_rare_data() - .slottable_data - .manual_slot_assignment = manually_assigned_slot.map(Dom::from_ref); - } } /// @@ -727,7 +690,6 @@ pub(crate) trait LayoutElementHelpers<'dom> { ) -> Option<&'dom AttrValue>; fn get_attr_val_for_layout(self, namespace: &Namespace, name: &LocalName) -> Option<&'dom str>; fn get_attr_vals_for_layout(self, name: &LocalName) -> Vec<&'dom AttrValue>; - fn get_assigned_slot(&self) -> Option>; } impl LayoutDom<'_, Element> { @@ -1261,20 +1223,6 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { }) .collect() } - - #[allow(unsafe_code)] - fn get_assigned_slot(&self) -> Option> { - unsafe { - self.unsafe_get() - .rare_data - .borrow_for_layout() - .as_ref()? - .slottable_data - .assigned_slot - .as_ref() - .map(|slot| slot.to_layout()) - } - } } impl Element { diff --git a/components/script/dom/eventtarget.rs b/components/script/dom/eventtarget.rs index 480aa1bb105..adf2cc0d118 100644 --- a/components/script/dom/eventtarget.rs +++ b/components/script/dom/eventtarget.rs @@ -41,7 +41,6 @@ use crate::dom::bindings::codegen::Bindings::NodeBinding::GetRootNodeOptions; use crate::dom::bindings::codegen::Bindings::NodeBinding::Node_Binding::NodeMethods; use crate::dom::bindings::codegen::Bindings::ShadowRootBinding::ShadowRoot_Binding::ShadowRootMethods; use crate::dom::bindings::codegen::Bindings::WindowBinding::WindowMethods; -use crate::dom::bindings::codegen::InheritTypes::{CharacterDataTypeId, NodeTypeId}; use crate::dom::bindings::codegen::UnionTypes::{ AddEventListenerOptionsOrBoolean, EventListenerOptionsOrBoolean, EventOrString, }; @@ -61,7 +60,6 @@ use crate::dom::globalscope::GlobalScope; use crate::dom::htmlformelement::FormControlElementHelpers; use crate::dom::node::{Node, NodeTraits}; use crate::dom::shadowroot::ShadowRoot; -use crate::dom::text::Text; use crate::dom::virtualmethods::VirtualMethods; use crate::dom::window::Window; use crate::dom::workerglobalscope::WorkerGlobalScope; @@ -821,25 +819,7 @@ impl EventTarget { if let Some(node) = self.downcast::() { // > A node’s get the parent algorithm, given an event, returns the node’s assigned slot, // > if node is assigned; otherwise node’s parent. - let assigned_slot = match node.type_id() { - NodeTypeId::Element(_) => { - let element = node.downcast::().unwrap(); - element - .assigned_slot() - .map(|slot| DomRoot::from_ref(slot.upcast::())) - }, - NodeTypeId::CharacterData(CharacterDataTypeId::Text(_)) => { - let text = node.downcast::().unwrap(); - text.slottable_data() - .borrow() - .assigned_slot - .as_ref() - .map(|slot| DomRoot::from_ref(slot.upcast::())) - }, - _ => None, - }; - - return assigned_slot.or_else(|| { + return node.assigned_slot().map(DomRoot::upcast).or_else(|| { node.GetParentNode() .map(|parent| DomRoot::from_ref(parent.upcast::())) }); diff --git a/components/script/dom/htmlslotelement.rs b/components/script/dom/htmlslotelement.rs index ccabca0972c..686705f9d4b 100644 --- a/components/script/dom/htmlslotelement.rs +++ b/components/script/dom/htmlslotelement.rs @@ -29,7 +29,6 @@ use crate::dom::globalscope::GlobalScope; use crate::dom::htmlelement::HTMLElement; use crate::dom::mutationobserver::MutationObserver; use crate::dom::node::{Node, NodeDamage, ShadowIncluding}; -use crate::dom::text::Text; use crate::dom::virtualmethods::VirtualMethods; use crate::script_runtime::CanGc; use crate::ScriptThread; @@ -437,53 +436,23 @@ impl Slottable { } pub(crate) fn assigned_slot(&self) -> Option> { - self.match_slottable( - |element: &Element| element.assigned_slot(), - |text: &Text| { - let assigned_slot = text - .slottable_data() - .borrow() - .assigned_slot - .as_ref()? - .as_rooted(); - Some(assigned_slot) - }, - ) + self.node().assigned_slot() } pub(crate) fn set_assigned_slot(&self, assigned_slot: Option<&HTMLSlotElement>) { - self.match_slottable( - |element: &Element| element.set_assigned_slot(assigned_slot), - |text: &Text| { - text.slottable_data().borrow_mut().assigned_slot = assigned_slot.map(Dom::from_ref); - }, - ) + self.node().set_assigned_slot(assigned_slot); } pub(crate) fn set_manual_slot_assignment( &self, manually_assigned_slot: Option<&HTMLSlotElement>, ) { - self.match_slottable( - |element: &Element| element.set_manual_slot_assignment(manually_assigned_slot), - |text: &Text| { - text.slottable_data().borrow_mut().manual_slot_assignment = - manually_assigned_slot.map(Dom::from_ref) - }, - ) + self.node() + .set_manual_slot_assignment(manually_assigned_slot); } pub(crate) fn manual_slot_assignment(&self) -> Option> { - self.match_slottable( - |element: &Element| element.manual_slot_assignment(), - |text: &Text| { - text.slottable_data() - .borrow() - .manual_slot_assignment - .as_ref() - .map(Dom::as_rooted) - }, - ) + self.node().manual_slot_assignment() } fn name(&self) -> DOMString { @@ -494,26 +463,6 @@ impl Slottable { element.get_string_attribute(&local_name!("slot")) } - - /// Call the `element_function` if the slottable is an Element, otherwise the - /// `text_function` - pub(crate) fn match_slottable(&self, element_function: E, text_function: T) -> R - where - E: FnOnce(&Element) -> R, - T: FnOnce(&Text) -> R, - { - match self.0.type_id() { - NodeTypeId::Element(_) => { - let element: &Element = self.0.downcast::().unwrap(); - element_function(element) - }, - NodeTypeId::CharacterData(CharacterDataTypeId::Text(_)) => { - let text: &Text = self.0.downcast::().unwrap(); - text_function(text) - }, - _ => unreachable!(), - } - } } impl VirtualMethods for HTMLSlotElement { diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 77be215cccb..b623ae18973 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1361,6 +1361,43 @@ impl Node { } } } + + pub(crate) fn assigned_slot(&self) -> Option> { + let assigned_slot = self + .rare_data + .borrow() + .as_ref()? + .slottable_data + .assigned_slot + .as_ref()? + .as_rooted(); + Some(assigned_slot) + } + + pub(crate) fn set_assigned_slot(&self, assigned_slot: Option<&HTMLSlotElement>) { + self.ensure_rare_data().slottable_data.assigned_slot = assigned_slot.map(Dom::from_ref); + } + + pub(crate) fn manual_slot_assignment(&self) -> Option> { + let manually_assigned_slot = self + .rare_data + .borrow() + .as_ref()? + .slottable_data + .manual_slot_assignment + .as_ref()? + .as_rooted(); + Some(manually_assigned_slot) + } + + pub(crate) fn set_manual_slot_assignment( + &self, + manually_assigned_slot: Option<&HTMLSlotElement>, + ) { + self.ensure_rare_data() + .slottable_data + .manual_slot_assignment = manually_assigned_slot.map(Dom::from_ref); + } } /// Iterate through `nodes` until we find a `Node` that is not in `not_in` @@ -1395,6 +1432,7 @@ pub(crate) trait LayoutNodeHelpers<'dom> { fn owner_doc_for_layout(self) -> LayoutDom<'dom, Document>; fn containing_shadow_root_for_layout(self) -> Option>; + fn assigned_slot_for_layout(self) -> Option>; fn is_element_for_layout(&self) -> bool; unsafe fn get_flag(self, flag: NodeFlags) -> bool; @@ -1517,6 +1555,21 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> { } } + #[inline] + #[allow(unsafe_code)] + fn assigned_slot_for_layout(self) -> Option> { + unsafe { + self.unsafe_get() + .rare_data + .borrow_for_layout() + .as_ref()? + .slottable_data + .assigned_slot + .as_ref() + .map(|assigned_slot| assigned_slot.to_layout()) + } + } + // FIXME(nox): get_flag/set_flag (especially the latter) are not safe because // they mutate stuff while values of this type can be used from multiple // threads at once, this should be revisited. @@ -2379,19 +2432,7 @@ impl Node { parent.remove_child(node, cached_index); // Step 12. If node is assigned, then run assign slottables for node’s assigned slot. - let assigned_slot = node - .downcast::() - .and_then(|e| e.assigned_slot()) - .or_else(|| { - node.downcast::().and_then(|text| { - text.slottable_data() - .borrow() - .assigned_slot - .as_ref() - .map(Dom::as_rooted) - }) - }); - if let Some(slot) = assigned_slot { + if let Some(slot) = node.assigned_slot() { slot.assign_slottables(); } diff --git a/components/script/dom/raredata.rs b/components/script/dom/raredata.rs index 383eaaf70c3..ac830a8e362 100644 --- a/components/script/dom/raredata.rs +++ b/components/script/dom/raredata.rs @@ -32,6 +32,8 @@ pub(crate) struct NodeRareData { pub(crate) mutation_observers: Vec, /// Lazily-generated Unique Id for this node. pub(crate) unique_id: Option, + + pub(crate) slottable_data: SlottableData, } #[derive(Default, JSTraceable, MallocSizeOf)] @@ -39,8 +41,6 @@ pub(crate) struct NodeRareData { pub(crate) struct ElementRareData { /// /// The ShadowRoot this element is host of. - /// XXX This is currently not exposed to web content. Only for - /// internal use. pub(crate) shadow_root: Option>, /// pub(crate) custom_element_reaction_queue: Vec, @@ -58,6 +58,4 @@ pub(crate) struct ElementRareData { pub(crate) client_rect: Option>>, /// pub(crate) element_internals: Option>, - - pub(crate) slottable_data: SlottableData, } diff --git a/components/script/dom/text.rs b/components/script/dom/text.rs index e34c9d2bd68..d4b31ab3dcf 100644 --- a/components/script/dom/text.rs +++ b/components/script/dom/text.rs @@ -2,8 +2,6 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use std::cell::RefCell; - use dom_struct::dom_struct; use js::rust::HandleObject; @@ -19,7 +17,7 @@ use crate::dom::bindings::str::DOMString; use crate::dom::characterdata::CharacterData; use crate::dom::document::Document; use crate::dom::globalscope::GlobalScope; -use crate::dom::htmlslotelement::{HTMLSlotElement, Slottable, SlottableData}; +use crate::dom::htmlslotelement::{HTMLSlotElement, Slottable}; use crate::dom::node::Node; use crate::dom::window::Window; use crate::script_runtime::CanGc; @@ -28,14 +26,12 @@ use crate::script_runtime::CanGc; #[dom_struct] pub(crate) struct Text { characterdata: CharacterData, - slottable_data: RefCell, } impl Text { pub(crate) fn new_inherited(text: DOMString, document: &Document) -> Text { Text { characterdata: CharacterData::new_inherited(text, document), - slottable_data: Default::default(), } } @@ -56,10 +52,6 @@ impl Text { can_gc, ) } - - pub(crate) fn slottable_data(&self) -> &RefCell { - &self.slottable_data - } } impl TextMethods for Text { diff --git a/components/script/layout_dom/element.rs b/components/script/layout_dom/element.rs index 05271793184..cdca829dca4 100644 --- a/components/script/layout_dom/element.rs +++ b/components/script/layout_dom/element.rs @@ -140,11 +140,6 @@ impl<'dom> ServoLayoutElement<'dom> { Some(node) => matches!(node.script_type_id(), NodeTypeId::Document(_)), } } - - fn assigned_slot(&self) -> Option { - let slot = self.element.get_assigned_slot()?; - Some(Self::from_layout_js(slot.upcast())) - } } pub enum DOMDescendantIterator @@ -204,11 +199,7 @@ impl<'dom> style::dom::TElement for ServoLayoutElement<'dom> { } fn traversal_parent(&self) -> Option { - if let Some(assigned_slot) = self.assigned_slot() { - Some(assigned_slot) - } else { - self.as_node().traversal_parent() - } + self.as_node().traversal_parent() } fn is_html_element(&self) -> bool { @@ -752,7 +743,7 @@ impl<'dom> ::selectors::Element for ServoLayoutElement<'dom> { #[allow(unsafe_code)] fn assigned_slot(&self) -> Option { - self.assigned_slot() + self.as_node().assigned_slot() } fn is_html_element_in_html_document(&self) -> bool { diff --git a/components/script/layout_dom/node.rs b/components/script/layout_dom/node.rs index 841a396fc5f..9e04e861844 100644 --- a/components/script/layout_dom/node.rs +++ b/components/script/layout_dom/node.rs @@ -94,6 +94,14 @@ impl<'dom> ServoLayoutNode<'dom> { pub(crate) fn get_jsmanaged(self) -> LayoutDom<'dom, Node> { self.node } + + pub(crate) fn assigned_slot(self) -> Option> { + self.node + .assigned_slot_for_layout() + .as_ref() + .map(LayoutDom::upcast) + .map(ServoLayoutElement::from_layout_js) + } } impl style::dom::NodeInfo for ServoLayoutNode<'_> { @@ -139,6 +147,9 @@ impl<'dom> style::dom::TNode for ServoLayoutNode<'dom> { } fn traversal_parent(&self) -> Option> { + if let Some(assigned_slot) = self.assigned_slot() { + return Some(assigned_slot); + } let parent = self.parent_node()?; if let Some(shadow) = parent.as_shadow_root() { return Some(shadow.host()); diff --git a/tests/unit/script/size_of.rs b/tests/unit/script/size_of.rs index 3cbc005ac53..d39f6ebacd7 100644 --- a/tests/unit/script/size_of.rs +++ b/tests/unit/script/size_of.rs @@ -35,5 +35,5 @@ sizeof_checker!(size_element, Element, 376); sizeof_checker!(size_htmlelement, HTMLElement, 392); sizeof_checker!(size_div, HTMLDivElement, 392); sizeof_checker!(size_span, HTMLSpanElement, 392); -sizeof_checker!(size_text, Text, 256); +sizeof_checker!(size_text, Text, 232); sizeof_checker!(size_characterdata, CharacterData, 232);