Fix segfauls when doing unwrap or decompose twice

Lexbor backend.

We now keep removed nodes in memory instead of deallocating them.
In consumes slightly more memory, but does not result in segfaults
when users try to remove same nodes twice.

Author: rushter

CHANGES.md

@@ -11,7 +11,9 @@
 - Optimize performance for`css_first` in lexbor backend
 - Fix segfaults when accessing attributes. Resolves #135.
 - Add new `.clone` method to lexbor backend. Resolve #117.
-- Improve unicode handling for malformed text. Resolved #138.
+- Improve unicode handling for malformed text. Resolves #138.
+- Fix segfaults when doing double `.decompose`. Resolves #179.
+- Fix sefgaults when doing double `.unwrap`. Resolves #169.
 
 ## Version 0.3.34
 

selectolax/lexbor.pyi

@@ -382,6 +382,8 @@ class LexborNode:
     def unwrap(self, delete_empty: bool = False) -> None:
         """Replace node with whatever is inside this node.
 
+        Does nothing if you perform unwrapping second time on the same node.
+
         Parameters
         ----------
         delete_empty : bool, default False

selectolax/lexbor.pyx

@@ -1,6 +1,7 @@
 from cpython.bool cimport bool
 from cpython.exc cimport PyErr_SetObject
 
+
 _ENCODING = 'UTF-8'
 
 include "base.pxi"
@@ -9,6 +10,7 @@ include "lexbor/attrs.pxi"
 include "lexbor/node.pxi"
 include "lexbor/selection.pxi"
 include "lexbor/util.pxi"
+include "lexbor/node_remove.pxi"
 
 # We don't inherit from HTMLParser here, because it also includes all the C code from Modest.
 

selectolax/lexbor/node.pxi

@@ -1,6 +1,8 @@
 cimport cython
 from cpython.exc cimport PyErr_SetNone
 
+import logging
+
 _TAG_TO_NAME = {
     0x0005: "- doctype",
     0x0002: "-text",
@@ -292,9 +294,9 @@ cdef class LexborNode:
             raise SelectolaxError("Decomposing the root node is not allowed.")
 
         if recursive:
-            lxb_dom_node_destroy_deep(<lxb_dom_node_t *> self.node)
+            node_remove_deep(<lxb_dom_node_t *> self.node)
         else:
-            lxb_dom_node_destroy(<lxb_dom_node_t *> self.node)
+            lxb_dom_node_remove(<lxb_dom_node_t *> self.node)
 
     def strip_tags(self, list tags, bool recursive = False):
         """Remove specified tags from the HTML tree.
@@ -438,6 +440,8 @@ cdef class LexborNode:
     def unwrap(self, bint delete_empty=False):
         """Replace node with whatever is inside this node.
 
+        Does nothing if you perform unwrapping second time on the same node.
+
         Parameters
         ----------
         delete_empty : bool, default False
@@ -453,9 +457,14 @@ cdef class LexborNode:
 
         Note: by default, empty tags are ignored, use "delete_empty" to change this.
         """
+
+        if node_is_removed(<lxb_dom_node_t *> self.node) == 1:
+            logging.error("Attempt to unwrap removed node. Does nothing.")
+            return
+
         if self.node.first_child == NULL:
             if delete_empty:
-                lxb_dom_node_destroy(<lxb_dom_node_t *> self.node)
+                lxb_dom_node_remove(<lxb_dom_node_t *> self.node)
             return
         cdef lxb_dom_node_t* next_node
         cdef lxb_dom_node_t* current_node
@@ -470,7 +479,7 @@ cdef class LexborNode:
                 current_node = next_node
         else:
             lxb_dom_node_insert_before(self.node, self.node.first_child)
-        lxb_dom_node_destroy(<lxb_dom_node_t *> self.node)
+        lxb_dom_node_remove(<lxb_dom_node_t *> self.node)
 
     def unwrap_tags(self, list tags, bint delete_empty = False):
         """Unwraps specified tags from the HTML tree.
@@ -610,7 +619,7 @@ cdef class LexborNode:
             if new_node == NULL:
                 raise SelectolaxError("Can't create a new node")
             lxb_dom_node_insert_before(self.node,  new_node)
-            lxb_dom_node_destroy(<lxb_dom_node_t *> self.node)
+            lxb_dom_node_remove(<lxb_dom_node_t *> self.node)
         elif isinstance(value, LexborNode):
             new_node = lxb_dom_document_import_node(
                 &self.parser.document.dom_document,
@@ -620,7 +629,7 @@ cdef class LexborNode:
             if new_node == NULL:
                 raise SelectolaxError("Can't create a new node")
             lxb_dom_node_insert_before(self.node, <lxb_dom_node_t *> new_node)
-            lxb_dom_node_destroy(<lxb_dom_node_t *> self.node)
+            lxb_dom_node_remove(<lxb_dom_node_t *> self.node)
         else:
             raise SelectolaxError("Expected a string or LexborNode instance, but %s found" % type(value).__name__)
 

selectolax/lexbor/node_remove.pxi

@@ -0,0 +1,29 @@
+
+cdef lxb_dom_node_t * node_remove_deep(lxb_dom_node_t* root):
+    cdef lxb_dom_node_t *tmp
+    cdef lxb_dom_node_t *node = root
+
+    while node != NULL:
+        if node.first_child != NULL:
+            node = node.first_child
+        else:
+            while node != root and node.next == NULL:
+                tmp = node.parent
+                lxb_dom_node_remove(node)
+                node = tmp
+
+            if node == root:
+                lxb_dom_node_remove(node)
+                break
+
+            tmp = node.next
+            lxb_dom_node_remove(node)
+            node = tmp
+
+    return NULL
+
+cdef bint node_is_removed(lxb_dom_node_t* node):
+    if node.parent == NULL and node.next == NULL \
+       and node.prev == NULL:
+        return 1
+    return 0

tests/test_lexbor.py

@@ -1,6 +1,6 @@
 """Tests for functionality that is only supported by lexbor backend."""
 
-from selectolax.lexbor import LexborHTMLParser
+from selectolax.lexbor import LexborHTMLParser, parse_fragment
 
 
 def test_reads_inner_html():
@@ -37,3 +37,16 @@ def test_node_cloning():
     new_node.inner_html = "<div>new</div>"
     assert parser.css_first("#main").html != new_node.html
     assert new_node.html == '<div id="main"><div>new</div></div>'
+
+
+def test_double_unwrap_does_not_segfault():
+    html = """<div><div><div></div></div></div>"""
+    outer_div = parse_fragment(html)[0]
+    some_set = set()
+
+    inner_div = outer_div.child
+    assert inner_div is not None
+    inner_div.unwrap()
+    inner_div.unwrap()
+    some_set.add(outer_div.parent)
+    some_set.add(outer_div.parent)