Never disconnect shadow roots from their hosts (#39459)

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 <simon.wuelker@arcor.de>

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) {

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>();
-        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");

components/script/dom/html/htmlmediaelement.rs

@@ -2111,9 +2111,9 @@ impl HTMLMediaElement {
         self.upcast::<Node>().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 {

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<Document>,
-    host: MutNullableDom<Element>,
+    host: Dom<Element>,
     /// List of author styles associated with nodes in this shadow tree.
     #[custom_trace]
     author_styles: DomRefCell<AuthorStyles<ServoStylesheetInDocument>>,
@@ -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>();
-        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::<Node>().dirty(NodeDamage::Style);
+        self.host.upcast::<Node>().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<crate::DomTypeHolder> for ShadowRoot {
 
     /// <https://dom.spec.whatwg.org/#dom-shadowroot-host>
     fn Host(&self) -> DomRoot<Element> {
-        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]

tests/wpt/mozilla/meta/MANIFEST.json

@@ -95,6 +95,15 @@
       {}
      ]
     ]
+   },
+   "shadow-dom": {
+    "move-element-with-ua-shadow-tree-crash.html": [
+     "c834ebc7bb778457b02aa6f4306498275eb956b0",
+     [
+      null,
+      {}
+     ]
+    ]
    }
   },
   "reftest": {

tests/wpt/mozilla/tests/shadow-dom/move-element-with-ua-shadow-tree-crash.html

@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<head>
+    <link rel="help" href="https://github.com/servo/servo/issues/36722">
+</head>
+<body>
+    <audio id="audio" controls src="data:video/mp4;base64,AAAAIGZ0eXBpc29tAAACAGlzb21pc28yYXZjMW1wNDEAAAAIZnJlZQAAA5NtZGF0AAACrgYF//+q3EXpvebZSLeWLNgg2SPu73gyNjQgLSBjb3JlIDE0OCByMjY0MyA1YzY1NzA0IC0gSC4yNjQvTVBFRy00IEFWQyBjb2RlYyAtIENvcHlsZWZ0IDIwMDMtMjAxNSAtIGh0dHA6Ly93d3cudmlkZW9sYW4ub3JnL3gyNjQuaHRtbCAtIG9wdGlvbnM6IGNhYmFjPTEgcmVmPTMgZGVibG9jaz0xOjA6MCBhbmFseXNlPTB4MzoweDExMyBtZT1oZXggc3VibWU9NyBwc3k9MSBwc3lfcmQ9MS4wMDowLjAwIG1peGVkX3JlZj0xIG1lX3JhbmdlPTE2IGNocm9tYV9tZT0xIHRyZWxsaXM9MSA4eDhkY3Q9MSBjcW09MCBkZWFkem9uZT0yMSwxMSBmYXN0X3Bza2lwPTEgY2hyb21hX3FwX29mZnNldD0tMiB0aHJlYWRzPTEgbG9va2FoZWFkX3RocmVhZHM9MSBzbGljZWRfdGhyZWFkcz0wIG5yPTAgZGVjaW1hdGU9MSBpbnRlcmxhY2VkPTAgYmx1cmF5X2NvbXBhdD0wIGNvbnN0cmFpbmVkX2ludHJhPTAgYmZyYW1lcz0zIGJfcHlyYW1pZD0yIGJfYWRhcHQ9MSBiX2JpYXM9MCBkaXJlY3Q9MSB3ZWlnaHRiPTEgb3Blbl9nb3A9MCB3ZWlnaHRwPTIga2V5aW50PTI1MCBrZXlpbnRfbWluPTI1IHNjZW5lY3V0PTQwIGludHJhX3JlZnJlc2g9MCByY19sb29rYWhlYWQ9NDAgcmM9Y3JmIG1idHJlZT0xIGNyZj0yMy4wIHFjb21wPTAuNjAgcXBtaW49MCBxcG1heD02OSBxcHN0ZXA9NCBpcF9yYXRpbz0xLjQwIGFxPTE6MS4wMACAAAAAvWWIhAAh/9PWYQ7q+jvvWOfBgvpv0eIYkqWiQW6SsLQx8ByoouBLEC9HBQTAXOJh/wFnteOP+NH5Er2DeHrP4kxvjj4nXKG9Zm/FycSAdlzoMDOFc4CmXmCL51Dj+zekurxKazOLwXVd7f/rOQpa9+iPXYTZsRw+WFFNokI8saLT7Mt03UvGxwdAYkwe7UmwPZacue5goP6rQhBgGMjgK21nSHZWUcz5Y6Ec/wdCPp0Sxx/h6UsSneF9hINuvwAAAAhBmiJsQx92QAAAAAgBnkF5DH/EgQAAAzRtb292AAAAbG12aGQAAAAAAAAAAAAAAAAAAAPoAAAAZAABAAABAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAACXnRyYWsAAABcdGtoZAAAAAMAAAAAAAAAAAAAAAEAAAAAAAAAZAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAEAAAAAAIAAAACAAAAAAACRlZHRzAAAAHGVsc3QAAAAAAAAAAQAAAGQAAAQAAAEAAAAAAdZtZGlhAAAAIG1kaGQAAAAAAAAAAAAAAAAAADwAAAAGAFXEAAAAAAAtaGRscgAAAAAAAAAAdmlkZQAAAAAAAAAAAAAAAFZpZGVvSGFuZGxlcgAAAAGBbWluZgAAABR2bWhkAAAAAQAAAAAAAAAAAAAAJGRpbmYAAAAcZHJlZgAAAAAAAAABAAAADHVybCAAAAABAAABQXN0YmwAAACVc3RzZAAAAAAAAAABAAAAhWF2YzEAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAIAAgAEgAAABIAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAY//8AAAAvYXZjQwFkAAr/4QAWZ2QACqzZSWhAAAADAEAAAA8DxIllgAEABmjr48siwAAAABhzdHRzAAAAAAAAAAEAAAADAAACAAAAABRzdHNzAAAAAAAAAAEAAAABAAAAKGN0dHMAAAAAAAAAAwAAAAEAAAQAAAAAAQAABgAAAAABAAACAAAAABxzdHNjAAAAAAAAAAEAAAABAAAAAwAAAAEAAAAgc3RzegAAAAAAAAAAAAAAAwAAA3MAAAAMAAAADAAAABRzdGNvAAAAAAAAAAEAAAAwAAAAYnVkdGEAAABabWV0YQAAAAAAAAAhaGRscgAAAAAAAAAAbWRpcmFwcGwAAAAAAAAAAAAAAAAtaWxzdAAAACWpdG9vAAAAHWRhdGEAAAABAAAAAExhdmY1Ni40MC4xMDE="></audio>
+    <script>
+    window.addEventListener("load", _ =>
+        document.createElement("div").prepend(audio));
+    </script>
+</body>