Revamps how libxml-ruby manages memory. Instead of trying to return t…

…he same ruby object for each xmlnode, the bindings now just create ruby objects as needed which are freed at the end of use. This allows most memory management to be handled by libxml itself. Ruby only hangs onto document objects and parent xml objects (not part of a document). When those go out of scope, the underlying libxml objects are also freed. default, the bindings now let libxml deal with freeing nodes
xml4r · Feb 8, 2017 · 2beb91d · 2beb91d
1 parent 9eba6ad
commit 2beb91d
Show file tree

Hide file tree

Showing 17 changed files with 246 additions and 361 deletions.
diff --git a/HISTORY b/HISTORY
@@ -1,5 +1,10 @@
 = Release History
 
+== 3.0.0 / 2017-02-06 Charlie Savage
+
+* Fix crash that sometimes happened when interating over a node's children (Charlie Savage)
+* Update Visual Studio project for Visual Studio 15 - requires ruby 2.4+ (Charlie Savage)
+
 == 2.9.0 / 2016-06-13 Charlie Savage
 
 * Revamp libxml-ruby's memory management to not cause crashes when used with Nokogiri (James Laird-Wah)

diff --git a/ext/libxml/ruby_xml.c b/ext/libxml/ruby_xml.c
@@ -1,112 +1,6 @@
 #include "ruby_libxml.h"
 #include "ruby_xml.h"
 
-static struct st_table *private_pointers;
-
-// Constant to track what nodes have been registered - this is just for performance (maybe not needed?)
-static int registered = 0;   
-
-void rxml_register(void *xnode, VALUE value)
-{
-  st_insert(private_pointers, (st_data_t)xnode, (st_data_t)value);
-}
-
-void rxml_register_node(xmlNodePtr xnode, VALUE value) 
-{
-  if (xnode->_private != &registered)
-  {
-    xnode->_private = &registered;
-    rxml_register(xnode, value);
-  }
-}
-
-void rxml_register_doc(xmlDocPtr xdoc, VALUE value)
-{
-  if (xdoc->_private != &registered)
-  {
-    xdoc->_private = &registered;
-    rxml_register(xdoc, value);
-  }
-}
-
-void rxml_register_dtd(xmlDtdPtr xdtd, VALUE value)
-{
-  if (xdtd->_private != &registered)
-  {
-    xdtd->_private = &registered;
-    rxml_register(xdtd, value);
-  }
-}
-
-void rxml_unregister(void *xnode)
-{
-  st_delete(private_pointers, (st_data_t*)&xnode, NULL);
-}
-
-void rxml_unregister_node(xmlNodePtr xnode)
-{
-  if (xnode->_private == &registered) 
-  {
-    xnode->_private = NULL;
-    rxml_unregister(xnode);
-  }
-}
-
-void rxml_unregister_doc(xmlDocPtr xdoc)
-{
-  if (xdoc->_private == &registered)
-  {
-    xdoc->_private = NULL;
-    rxml_unregister(xdoc);
-  }
-}
-
-void rxml_unregister_dtd(xmlDtdPtr xdtd)
-{
-  if (xdtd->_private == &registered)
-  {
-    xdtd->_private = NULL;
-    rxml_unregister(xdtd);
-  }
-}
-
-VALUE rxml_lookup(void *pointer)
-{
-  VALUE result = Qnil;
-
-  st_data_t data_t = 0;
-  if (st_lookup(private_pointers, (st_data_t)pointer, &data_t))
-  {
-    result = (VALUE)data_t;
-  }
-
-  return result;
-}
-
-VALUE rxml_lookup_node(xmlNodePtr xnode)
-{
-  if (!xnode || xnode->_private != &registered)
-    return Qnil;
-
-  return rxml_lookup(xnode);
-}
-
-VALUE rxml_lookup_doc(xmlDocPtr xdoc)
-{
-  if (!xdoc || xdoc->_private != &registered)
-    return Qnil;
-
-  return rxml_lookup(xdoc);
-}
-
-VALUE rxml_lookup_dtd(xmlDtdPtr xdtd)
-{
-  if (!xdtd || xdtd->_private != &registered)
-    return Qnil;
-
-  return rxml_lookup(xdtd);
-}
-
 VALUE mXML;
 
 /*
@@ -940,9 +834,6 @@ static VALUE rxml_memory_used(VALUE self)
 
 void rxml_init_xml(void)
 {
-  /* Create a hashtable suitable for pointer keys */
-  private_pointers = st_init_numtable();
-
   mXML = rb_define_module_under(mLibXML, "XML");
 
   /* Constants */

diff --git a/ext/libxml/ruby_xml.h b/ext/libxml/ruby_xml.h
@@ -7,14 +7,4 @@ extern VALUE mXML;
 int rxml_libxml_default_options();
 void rxml_init_xml(void);
 
-void rxml_register_node(xmlNodePtr node, VALUE value);
-void rxml_register_doc(xmlDocPtr doc, VALUE value);
-void rxml_register_dtd(xmlDtdPtr dtd, VALUE value);
-void rxml_unregister_node(xmlNodePtr node);
-void rxml_unregister_doc(xmlDocPtr doc);
-void rxml_unregister_dtd(xmlDtdPtr dtd);
-VALUE rxml_lookup_node(xmlNodePtr node);
-VALUE rxml_lookup_doc(xmlDocPtr doc);
-VALUE rxml_lookup_dtd(xmlDtdPtr dtd);
-
 #endif
diff --git a/ext/libxml/ruby_xml_document.c b/ext/libxml/ruby_xml_document.c
@@ -54,39 +54,27 @@
 
 VALUE cXMLDocument;
 
-
-void rxml_document_mark_node_list(xmlNodePtr xnode)
-{
-  if (xnode == NULL) return;
-
-  while (xnode != NULL)
-  {
-    rxml_document_mark_node_list(xnode->children);
-    rxml_node_mark(xnode);
-    xnode = xnode->next;
-  }
-}
-
-void rxml_document_mark(xmlDocPtr xdoc)
-{
-  if (xdoc)
-    rxml_document_mark_node_list(xdoc->children);
-}
-
 void rxml_document_free(xmlDocPtr xdoc)
 {
-  rxml_unregister_doc(xdoc);
+  xdoc->_private = NULL;
   xmlFreeDoc(xdoc);
 }
 
 VALUE rxml_document_wrap(xmlDocPtr xdoc)
 {
-  VALUE result = rxml_lookup_doc(xdoc);
+  VALUE result = Qnil;
 
-  if (result == Qnil) {
-    result = Data_Wrap_Struct(cXMLDocument, rxml_document_mark, rxml_document_free, xdoc);
-    rxml_register_doc(xdoc, result);
+  // Is this node is already wrapped?
+  if (xdoc->_private != NULL)
+  {
+    result = (VALUE)xdoc->_private;
+  }
+  else
+  {
+    result = Data_Wrap_Struct(cXMLDocument, NULL, rxml_document_free, xdoc);
+    xdoc->_private = (void*)result;
   }
+
   return result;
 }
 
@@ -99,7 +87,7 @@ VALUE rxml_document_wrap(xmlDocPtr xdoc)
  */
 static VALUE rxml_document_alloc(VALUE klass)
 {
-  return Data_Wrap_Struct(klass, rxml_document_mark, rxml_document_free, NULL);
+  return Data_Wrap_Struct(klass, NULL, rxml_document_free, NULL);
 }
 
 /*
@@ -128,8 +116,10 @@ static VALUE rxml_document_initialize(int argc, VALUE *argv, VALUE self)
 
   Check_Type(xmlver, T_STRING);
   xdoc = xmlNewDoc((xmlChar*) StringValuePtr(xmlver));
-  rxml_register_doc(xdoc, self);
-  DATA_PTR(self) = xdoc;
+
+  // Link the ruby object to the document and the document to the ruby object
+  RDATA(self)->data = xdoc;
+  xdoc->_private = (void*)self;
 
   return self;
 }
@@ -740,6 +730,7 @@ static VALUE rxml_document_root_set(VALUE self, VALUE node)
 {
   xmlDocPtr xdoc;
   xmlNodePtr xnode;
+  xmlNodePtr xOldRoot;
 
   if (rb_obj_is_kind_of(node, cXMLNode) == Qfalse)
     rb_raise(rb_eTypeError, "must pass an XML::Node type object");
@@ -750,7 +741,11 @@ static VALUE rxml_document_root_set(VALUE self, VALUE node)
   if (xnode->doc != NULL && xnode->doc != xdoc)
     rb_raise(eXMLError, "Nodes belong to different documents.  You must first import the node by calling XML::Document.import");
 
-  xmlDocSetRootElement(xdoc, xnode);
+  xOldRoot = xmlDocSetRootElement(xdoc, xnode);
+
+  // Ruby no longer manages this nodes memory
+  rxml_node_unmanage(xnode, node);
+
   return node;
 }
 

diff --git a/ext/libxml/ruby_xml_dtd.c b/ext/libxml/ruby_xml_dtd.c
@@ -32,10 +32,6 @@ VALUE cXMLDtd;
 
 void rxml_dtd_free(xmlDtdPtr xdtd)
 {
-  /* Clear our private pointer so that we won't reuse the
-   same, freed, Ruby wrapper object later.*/
-  rxml_unregister_dtd(xdtd);
-
   if (xdtd->doc == NULL && xdtd->parent == NULL)
     xmlFreeDtd(xdtd);
 }
@@ -47,7 +43,7 @@ void rxml_dtd_mark(xmlDtdPtr xdtd)
   if (xdtd == NULL)
     return;
 
-  doc = rxml_lookup_doc(xdtd->doc);
+  doc = (VALUE)xdtd->doc->_private;
   rb_gc_mark(doc);
 }
 
@@ -58,14 +54,7 @@ static VALUE rxml_dtd_alloc(VALUE klass)
 
 VALUE rxml_dtd_wrap(xmlDtdPtr xdtd)
 {
-  VALUE result = rxml_lookup_dtd(xdtd);
-
-  // This node is already wrapped
-  if (result == Qnil) {
-    result = Data_Wrap_Struct(cXMLDtd, NULL, NULL, xdtd);
-    rxml_register_dtd(xdtd, result);
-  }
-  return result;
+  return  Data_Wrap_Struct(cXMLDtd, NULL, NULL, xdtd);
 }
 
 /*