rcdom's node's destructor clears all children for nodes that can have strong references outside of the tree

[jdm]

#409 contains this code:

            // If this is not the only strong reference, don't drop any children, because
            // apparently somebody else wants them. Just let our reference drop instead.
            if Rc::strong_count(&root) > 1 {
                continue;
            }

Our current implementation does not perform this check, but presumably this isn't a case that affects our tests which are largely concerned with the tree structure.

[asquared31415]

This can definitely cause weird and hard to debug behavior. I ran into a case in practice where a parent node can be dropped but a child node might still have a strong reference. This caused usage of the child node to be incorrect as that node's children had been removed by the current Drop impl. I believe that simply changing line 133 which extends the list of nodes to remove to something like nodes.extend(children.into_iter().filter(|node| Rc::strong_count(node) <= 1)) would fix this issue.

[jdm]

Per the description in https://github.com/servo/html5ever/tree/master/rcdom#readme, I recommend forking rcdom to make whatever changes you want to it.

[skanfd]

So why is there a manually implemented Drop for Node ? I've thought Node and it's children will be automatically dropped when strong count reach zero. And these codes just flatten the tree?

impl Drop for Node {
    fn drop(&mut self) {
        let mut nodes = mem::replace(&mut *self.children.borrow_mut(), vec![]);
        while let Some(node) = nodes.pop() {
            let children = mem::replace(&mut *node.children.borrow_mut(), vec![]);
            nodes.extend(children.into_iter());
            if let NodeData::Element { ref template_contents, .. } = node.data {
                if let Some(template_contents) = template_contents.borrow_mut().take() {
                    nodes.push(template_contents);
                }
            }
        }
    }
}

[froydnj]

So why is there a manually implemented Drop for Node ?

So that the children are dropped iteratively, not recursively. See PR #384.

[skanfd]

Thank you for your time! @froydnj