html5ever
html5ever copied to clipboard
rcdom's node's destructor clears all children for nodes that can have strong references outside of the tree
#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.
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.
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.
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);
}
}
}
}
}
So why is there a manually implemented
Drop
forNode
?
So that the children are dropped iteratively, not recursively. See PR #384.
Thank you for your time! @froydnj