Correctly handle xmlNs memory in xpath query results #1362
Conversation
to stop freeing the namespace nodes when the NodeSet is GCed. Related to #1155
to remove nokogiriNodeSetTuple which is no longer necessary Related to #1155
This allows us to move repeated NULL checks out of callers.
introduce Check_Node_Set_Node_Type to replace repeated logic.
previously we had one code path (in #minus) that might try to free xmlNs structs in the xmlNodeset.
this commit introduces a bifurcation in how Namespace objects are managed; Namespace objects wrapped around xmlNs structs from an xpath query now have their own Ruby object lifecycle and are GCed independently from their original document. See comments in xml_node_set.c and xml_namespace.c for more details. Related to #1155
| Data_Get_Struct(self, xmlNodeSet, node_set); | ||
| Data_Get_Struct(rb_node, xmlNode, node); | ||
|
|
||
| if (xmlXPathNodeSetContains(node_set, node)) { |
There was a problem hiding this comment.
Looks like this condition is unnecessary now, since xpath_node_set_del() does the same check internally.
There was a problem hiding this comment.
Well, the return value changes, and so we need to know whether the object is in the node set or not.
We could have xpath_node_set_del return a boolean (found or not-found), but honestly I'll take the performance hit in exchange for more readable code at this point, given how complicated namespace handling is.
It's rare that namespaces are returned in xpath queries anyway, and so I'd prefer to leave it as-is.
|
Great work @flavorjones ! Looks like a correct solution to the GC issue. For sure much better than the previous way per |
|
Merged manually (with conflicts). |
This patch addresses #1155
This patch introduces a bifurcation in how Namespace objects are managed; Namespace objects wrapped around xmlNs structs from an xpath query now have their own Ruby object lifecycle and are GCed independently from their original document.
See commits and comments in xml_node_set.c and xml_namespace.c for more details.
Shout out to @akshaysawant who helped diagnose the underlying issue.
Also note that this is clean under valgrind, and doesn't leak memory related to namespace xpath queries.
(This replaces #1360 and is rebased on a green commit.)