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

Activity

1. asquared31415 commented on May 2, 2021

   

   asquared31415

   on May 2, 2021

   More actions

   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.
2. jdm commented on May 2, 2021

   

   jdm

   on May 2, 2021

   MemberAuthor

   More actions

   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.
3. skanfd commented on Feb 14, 2022

   

   skanfd

   on Feb 14, 2022

   More actions

   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);
                   }
               }
           }
       }
   }
4. froydnj commented on Feb 14, 2022

   

   froydnj

   on Feb 14, 2022

   More actions

   So why is there a manually implemented Drop for Node ?

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

   👍React with 👍1Reacted by skanfd
5. skanfd commented on Feb 14, 2022

   

   skanfd

   on Feb 14, 2022

   More actions

   Thank you for your time! @froydnj