From 72555de341ebc54d27d8462288f6b80e27dc713c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20W=C3=BClker?= Date: Wed, 24 Sep 2025 10:04:56 +0200 Subject: [PATCH] Never disconnect shadow roots from their hosts (#39459) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regular shadow roots can never be detached once they are created. However we were specifically detaching shadow roots from media elements when they were disconnected. This is actually one of the very few aspects of shadow roots that predate the implementation work I did earlier this year. I'm not sure why we ever did this. Maybe its for efficiency reasons, because keeping the shadow tree around is not necessary when the media element is not connected. But I can't imagine this yields any benefits, especially since you would have to reconstruct the shadow tree when the media element is re-connected (as is the case in the crash we observe). For simplicities sake, I have completely removed this functionality. Doing so ends up simplifying the code quite a bit. Testing: This change adds a new crashtest Fixes https://github.com/servo/servo/issues/36722 --------- Signed-off-by: Simon Wülker --- components/script/dom/document.rs | 14 ++++---- components/script/dom/element.rs | 13 ------- .../script/dom/html/htmlmediaelement.rs | 8 ++--- components/script/dom/shadowroot.rs | 36 ++++++------------- tests/wpt/mozilla/meta/MANIFEST.json | 9 +++++ ...ove-element-with-ua-shadow-tree-crash.html | 11 ++++++ 6 files changed, 40 insertions(+), 51 deletions(-) create mode 100644 tests/wpt/mozilla/tests/shadow-dom/move-element-with-ua-shadow-tree-crash.html diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index b97e3b3e052..bf5ce902a31 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -95,7 +95,6 @@ use crate::dom::bindings::codegen::Bindings::NodeBinding::NodeMethods; use crate::dom::bindings::codegen::Bindings::NodeFilterBinding::NodeFilter; use crate::dom::bindings::codegen::Bindings::PerformanceBinding::PerformanceMethods; use crate::dom::bindings::codegen::Bindings::PermissionStatusBinding::PermissionName; -use crate::dom::bindings::codegen::Bindings::ShadowRootBinding::ShadowRootMethods; use crate::dom::bindings::codegen::Bindings::WindowBinding::{ FrameRequestCallback, ScrollBehavior, ScrollOptions, WindowMethods, }; @@ -2633,13 +2632,12 @@ impl Document { id } - pub(crate) fn unregister_media_controls(&self, id: &str, can_gc: CanGc) { - if let Some(ref media_controls) = self.media_controls.borrow_mut().remove(id) { - let media_controls = DomRoot::from_ref(&**media_controls); - media_controls.Host().detach_shadow(can_gc); - } else { - debug_assert!(false, "Trying to unregister unknown media controls"); - } + pub(crate) fn unregister_media_controls(&self, id: &str) { + let did_have_these_media_controls = self.media_controls.borrow_mut().remove(id).is_some(); + debug_assert!( + did_have_these_media_controls, + "Trying to unregister unknown media controls" + ); } pub(crate) fn add_dirty_webgl_canvas(&self, context: &WebGLRenderingContext) { diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 07a46384e8e..61a7ef7beb2 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -730,19 +730,6 @@ impl Element { root } - pub(crate) fn detach_shadow(&self, can_gc: CanGc) { - let Some(ref shadow_root) = self.shadow_root() else { - unreachable!("Trying to detach a non-attached shadow root"); - }; - - let node = self.upcast::(); - node.note_dirty_descendants(); - node.rev_version(); - - shadow_root.detach(can_gc); - self.ensure_rare_data().shadow_root = None; - } - // https://html.spec.whatwg.org/multipage/#translation-mode pub(crate) fn is_translate_enabled(&self) -> bool { let name = &local_name!("translate"); diff --git a/components/script/dom/html/htmlmediaelement.rs b/components/script/dom/html/htmlmediaelement.rs index f9d8becfce5..f59d637e140 100644 --- a/components/script/dom/html/htmlmediaelement.rs +++ b/components/script/dom/html/htmlmediaelement.rs @@ -2111,9 +2111,9 @@ impl HTMLMediaElement { self.upcast::().dirty(NodeDamage::Other); } - fn remove_controls(&self, can_gc: CanGc) { + fn remove_controls(&self) { if let Some(id) = self.media_controls_id.borrow_mut().take() { - self.owner_document().unregister_media_controls(&id, can_gc); + self.owner_document().unregister_media_controls(&id); } } @@ -2696,7 +2696,7 @@ impl VirtualMethods for HTMLMediaElement { if mutation.new_value(attr).is_some() { self.render_controls(can_gc); } else { - self.remove_controls(can_gc); + self.remove_controls(); } }, _ => (), @@ -2707,7 +2707,7 @@ impl VirtualMethods for HTMLMediaElement { fn unbind_from_tree(&self, context: &UnbindContext, can_gc: CanGc) { self.super_type().unwrap().unbind_from_tree(context, can_gc); - self.remove_controls(can_gc); + self.remove_controls(); if context.tree_connected { let task = MediaElementMicrotask::PauseIfNotInDocument { diff --git a/components/script/dom/shadowroot.rs b/components/script/dom/shadowroot.rs index c27e2487c42..44c8cca799c 100644 --- a/components/script/dom/shadowroot.rs +++ b/components/script/dom/shadowroot.rs @@ -35,7 +35,7 @@ use crate::dom::bindings::frozenarray::CachedFrozenArray; use crate::dom::bindings::inheritance::Castable; use crate::dom::bindings::num::Finite; use crate::dom::bindings::reflector::reflect_dom_object; -use crate::dom::bindings::root::{Dom, DomRoot, LayoutDom, MutNullableDom}; +use crate::dom::bindings::root::{Dom, DomRoot, LayoutDom, MutNullableDom, ToLayout}; use crate::dom::bindings::str::DOMString; use crate::dom::cssstylesheet::CSSStyleSheet; use crate::dom::document::Document; @@ -70,7 +70,7 @@ pub(crate) struct ShadowRoot { document_fragment: DocumentFragment, document_or_shadow_root: DocumentOrShadowRoot, document: Dom, - host: MutNullableDom, + host: Dom, /// List of author styles associated with nodes in this shadow tree. #[custom_trace] author_styles: DomRefCell>, @@ -133,7 +133,7 @@ impl ShadowRoot { document_fragment, document_or_shadow_root: DocumentOrShadowRoot::new(document.window()), document: Dom::from_ref(document), - host: MutNullableDom::new(Some(host)), + host: Dom::from_ref(host), author_styles: DomRefCell::new(AuthorStyles::new()), stylesheet_list: MutNullableDom::new(None), window: Dom::from_ref(document.window()), @@ -174,14 +174,6 @@ impl ShadowRoot { ) } - pub(crate) fn detach(&self, can_gc: CanGc) { - self.document.unregister_shadow_root(self); - let node = self.upcast::(); - node.set_containing_shadow_root(None); - Node::complete_remove_subtree(node, &UnbindContext::new(node, None, None, None), can_gc); - self.host.set(None); - } - pub(crate) fn owner_doc(&self) -> &Document { &self.document } @@ -269,14 +261,12 @@ impl ShadowRoot { self.document.invalidate_shadow_roots_stylesheets(); self.author_styles.borrow_mut().stylesheets.force_dirty(); // Mark the host element dirty so a reflow will be performed. - if let Some(host) = self.host.get() { - host.upcast::().dirty(NodeDamage::Style); + self.host.upcast::().dirty(NodeDamage::Style); - // Also mark the host element with `RestyleHint::restyle_subtree` so a reflow - // can traverse into the shadow tree. - let mut restyle = self.document.ensure_pending_restyle(&host); - restyle.hint.insert(RestyleHint::restyle_subtree()); - } + // Also mark the host element with `RestyleHint::restyle_subtree` so a reflow + // can traverse into the shadow tree. + let mut restyle = self.document.ensure_pending_restyle(&self.host); + restyle.hint.insert(RestyleHint::restyle_subtree()); } /// Remove any existing association between the provided id and any elements @@ -441,8 +431,7 @@ impl ShadowRootMethods for ShadowRoot { /// fn Host(&self) -> DomRoot { - let host = self.host.get(); - host.expect("Trying to get host from a detached shadow root") + self.host.as_rooted() } // https://drafts.csswg.org/cssom/#dom-document-stylesheets @@ -625,12 +614,7 @@ impl<'dom> LayoutShadowRootHelpers<'dom> for LayoutDom<'dom, ShadowRoot> { #[inline] #[allow(unsafe_code)] fn get_host_for_layout(self) -> LayoutDom<'dom, Element> { - unsafe { - self.unsafe_get() - .host - .get_inner_as_layout() - .expect("We should never do layout on a detached shadow root") - } + unsafe { self.unsafe_get().host.to_layout() } } #[inline] diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 8ffa3208e93..b0dff6440fd 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -95,6 +95,15 @@ {} ] ] + }, + "shadow-dom": { + "move-element-with-ua-shadow-tree-crash.html": [ + "c834ebc7bb778457b02aa6f4306498275eb956b0", + [ + null, + {} + ] + ] } }, "reftest": { diff --git a/tests/wpt/mozilla/tests/shadow-dom/move-element-with-ua-shadow-tree-crash.html b/tests/wpt/mozilla/tests/shadow-dom/move-element-with-ua-shadow-tree-crash.html new file mode 100644 index 00000000000..c834ebc7bb7 --- /dev/null +++ b/tests/wpt/mozilla/tests/shadow-dom/move-element-with-ua-shadow-tree-crash.html @@ -0,0 +1,11 @@ + + + + + + + +