Moar detach shadow improvements

components/layout_thread/dom_wrapper.rs

@@ -189,9 +189,7 @@ impl<'lr> TShadowRoot for ServoShadowRoot<'lr> {
     }
 
     fn host(&self) -> ServoLayoutElement<'lr> {
-        ServoLayoutElement::from_layout_js(unsafe {
-            self.shadow_root.get_host_for_layout().unwrap()
-        })
+        ServoLayoutElement::from_layout_js(unsafe { self.shadow_root.get_host_for_layout() })
     }
 
     fn style_data<'a>(&self) -> Option<&'a CascadeData>

components/script/dom/node.rs

@@ -299,6 +299,36 @@ impl Node {
         }
     }
 
+    /// Clean up flags and unbind from tree.
+    pub fn complete_remove_subtree(root: &Node, context: &UnbindContext) {
+        for node in root.traverse_preorder(ShadowIncluding::Yes) {
+            // Out-of-document elements never have the descendants flag set.
+            node.set_flag(
+                NodeFlags::IS_IN_DOC |
+                    NodeFlags::IS_CONNECTED |
+                    NodeFlags::HAS_DIRTY_DESCENDANTS |
+                    NodeFlags::HAS_SNAPSHOT |
+                    NodeFlags::HANDLED_SNAPSHOT,
+                false,
+            );
+        }
+        for node in root.traverse_preorder(ShadowIncluding::Yes) {
+            // This needs to be in its own loop, because unbind_from_tree may
+            // rely on the state of IS_IN_DOC of the context node's descendants,
+            // e.g. when removing a <form>.
+            vtable_for(&&*node).unbind_from_tree(&context);
+            node.style_and_layout_data.get().map(|d| node.dispose(d));
+            // https://dom.spec.whatwg.org/#concept-node-remove step 14
+            if let Some(element) = node.as_custom_element() {
+                ScriptThread::enqueue_callback_reaction(
+                    &*element,
+                    CallbackReaction::Disconnected,
+                    None,
+                );
+            }
+        }
+    }
+
     /// Removes the given child from this node's list of children.
     ///
     /// Fails unless `child` is a child of this node.
@@ -339,32 +369,7 @@ impl Node {
         child.parent_node.set(None);
         self.children_count.set(self.children_count.get() - 1);
 
-        for node in child.traverse_preorder(ShadowIncluding::Yes) {
-            // Out-of-document elements never have the descendants flag set.
-            node.set_flag(
-                NodeFlags::IS_IN_DOC |
-                    NodeFlags::IS_CONNECTED |
-                    NodeFlags::HAS_DIRTY_DESCENDANTS |
-                    NodeFlags::HAS_SNAPSHOT |
-                    NodeFlags::HANDLED_SNAPSHOT,
-                false,
-            );
-        }
-        for node in child.traverse_preorder(ShadowIncluding::Yes) {
-            // This needs to be in its own loop, because unbind_from_tree may
-            // rely on the state of IS_IN_DOC of the context node's descendants,
-            // e.g. when removing a <form>.
-            vtable_for(&&*node).unbind_from_tree(&context);
-            node.style_and_layout_data.get().map(|d| node.dispose(d));
-            // https://dom.spec.whatwg.org/#concept-node-remove step 14
-            if let Some(element) = node.as_custom_element() {
-                ScriptThread::enqueue_callback_reaction(
-                    &*element,
-                    CallbackReaction::Disconnected,
-                    None,
-                );
-            }
-        }
+        Self::complete_remove_subtree(child, &context);
     }
 
     pub fn to_untrusted_node_address(&self) -> UntrustedNodeAddress {
@@ -962,7 +967,7 @@ impl Node {
     }
 
     pub fn set_containing_shadow_root(&self, shadow_root: Option<&ShadowRoot>) {
-        self.ensure_rare_data().containing_shadow_root = shadow_root.map(|sr| Dom::from_ref(sr));
+        self.ensure_rare_data().containing_shadow_root = shadow_root.map(Dom::from_ref);
     }
 
     pub fn is_in_html_doc(&self) -> bool {
@@ -1241,7 +1246,7 @@ impl LayoutNodeHelpers for LayoutDom<Node> {
         let parent = (*self.unsafe_get()).parent_node.get_inner_as_layout();
         if let Some(ref parent) = parent {
             if let Some(shadow_root) = parent.downcast::<ShadowRoot>() {
-                return shadow_root.get_host_for_layout().map(|h| h.upcast());
+                return Some(shadow_root.get_host_for_layout().upcast());
             }
         }
         parent
@@ -3082,7 +3087,7 @@ pub struct UnbindContext<'a> {
 
 impl<'a> UnbindContext<'a> {
     /// Create a new `UnbindContext` value.
-    fn new(
+    pub fn new(
         parent: &'a Node,
         prev_sibling: Option<&'a Node>,
         next_sibling: Option<&'a Node>,

components/script/dom/shadowroot.rs

@@ -3,7 +3,6 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 use crate::dom::bindings::cell::DomRefCell;
-use crate::dom::bindings::codegen::Bindings::NodeBinding::NodeMethods;
 use crate::dom::bindings::codegen::Bindings::ShadowRootBinding::ShadowRootBinding::ShadowRootMethods;
 use crate::dom::bindings::codegen::Bindings::ShadowRootBinding::{self, ShadowRootMode};
 use crate::dom::bindings::inheritance::Castable;
@@ -15,7 +14,7 @@ use crate::dom::document::Document;
 use crate::dom::documentfragment::DocumentFragment;
 use crate::dom::documentorshadowroot::{DocumentOrShadowRoot, StyleSheetInDocument};
 use crate::dom::element::Element;
-use crate::dom::node::{Node, NodeDamage, NodeFlags, ShadowIncluding};
+use crate::dom::node::{Node, NodeDamage, NodeFlags, ShadowIncluding, UnbindContext};
 use crate::dom::stylesheetlist::{StyleSheetList, StyleSheetListOwner};
 use crate::dom::window::Window;
 use crate::stylesheet_set::StylesheetSetRef;
@@ -82,21 +81,7 @@ impl ShadowRoot {
         self.document.unregister_shadow_root(&self);
         let node = self.upcast::<Node>();
         node.set_containing_shadow_root(None);
-        for child in node.traverse_preorder(ShadowIncluding::No) {
-            if node.RemoveChild(&child).is_err() {
-                warn!("Could not remove shadow root child");
-            }
-        }
-        if self
-            .host
-            .get()
-            .unwrap()
-            .upcast::<Node>()
-            .RemoveChild(&node)
-            .is_err()
-        {
-            warn!("Could not detach shadow root");
-        }
+        Node::complete_remove_subtree(&node, &UnbindContext::new(node, None, None, None));
         self.host.set(None);
     }
 
@@ -240,8 +225,7 @@ impl ShadowRootMethods for ShadowRoot {
     /// https://dom.spec.whatwg.org/#dom-shadowroot-host
     fn Host(&self) -> DomRoot<Element> {
         let host = self.host.get();
-        debug_assert!(host.is_some());
-        host.unwrap()
+        host.expect("Trying to get host from a detached shadow root")
     }
 
     // https://drafts.csswg.org/cssom/#dom-document-stylesheets
@@ -257,7 +241,7 @@ impl ShadowRootMethods for ShadowRoot {
 
 #[allow(unsafe_code)]
 pub trait LayoutShadowRootHelpers {
-    unsafe fn get_host_for_layout(&self) -> Option<LayoutDom<Element>>;
+    unsafe fn get_host_for_layout(&self) -> LayoutDom<Element>;
     unsafe fn get_style_data_for_layout<'a, E: TElement>(
         &self,
     ) -> &'a AuthorStyles<StyleSheetInDocument>;
@@ -272,8 +256,11 @@ pub trait LayoutShadowRootHelpers {
 impl LayoutShadowRootHelpers for LayoutDom<ShadowRoot> {
     #[inline]
     #[allow(unsafe_code)]
-    unsafe fn get_host_for_layout(&self) -> Option<LayoutDom<Element>> {
-        (*self.unsafe_get()).host.get_inner_as_layout()
+    unsafe fn get_host_for_layout(&self) -> LayoutDom<Element> {
+        (*self.unsafe_get())
+            .host
+            .get_inner_as_layout()
+            .expect("We should never do layout on a detached shadow root")
     }
 
     #[inline]