Skip to content

Commit 4db3b4d

Browse files
flavorjonesflavorjones
authored
Merge pull request #2668 from sparklemotion/flavorjones-namespace-scopes-compaction_v1.13.x
fix: namespace nodes behave with compaction (backport to v1.13.x)
2 parents 7b369e5 + 73d73d6 commit 4db3b4d

File tree

6 files changed

+143
-17
lines changed

6 files changed

+143
-17
lines changed

‎ext/nokogiri/nokogiri.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ int noko_io_write(void *ctx, char *buffer, int len);
171171
int noko_io_close(void *ctx);
172172

173173
#define Noko_Node_Get_Struct(obj,type,sval) ((sval) = (type*)DATA_PTR(obj))
174+
#define Noko_Namespace_Get_Struct(obj,type,sval) ((sval) = (type*)DATA_PTR(obj))
174175

175176
VALUE noko_xml_node_wrap(VALUE klass, xmlNodePtr node) ;
176177
VALUE noko_xml_node_wrap_node_set_result(xmlNodePtr node, VALUE node_set) ;

‎ext/nokogiri/xml_document.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,11 @@ recursively_remove_namespaces_from_node(xmlNodePtr node)
104104
(node->type == XML_XINCLUDE_START) ||
105105
(node->type == XML_XINCLUDE_END)) &&
106106
node->nsDef) {
107-
xmlFreeNsList(node->nsDef);
107+
xmlNsPtr curr = node->nsDef;
108+
while (curr) {
109+
noko_xml_document_pin_namespace(curr, node->doc);
110+
curr = curr->next;
111+
}
108112
node->nsDef = NULL;
109113
}
110114

‎ext/nokogiri/xml_namespace.c

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,15 @@
2525
VALUE cNokogiriXmlNamespace ;
2626

2727
static void
28-
dealloc_namespace(xmlNsPtr ns)
28+
_xml_namespace_dealloc(void *ptr)
2929
{
3030
/*
3131
* this deallocator is only used for namespace nodes that are part of an xpath
3232
* node set. see noko_xml_namespace_wrap().
3333
*/
34+
xmlNsPtr ns = ptr;
3435
NOKOGIRI_DEBUG_START(ns) ;
36+
3537
if (ns->href) {
3638
xmlFree(DISCARD_CONST_QUAL_XMLCHAR(ns->href));
3739
}
@@ -42,6 +44,36 @@ dealloc_namespace(xmlNsPtr ns)
4244
NOKOGIRI_DEBUG_END(ns) ;
4345
}
4446

47+
#ifdef HAVE_RB_GC_LOCATION
48+
static void
49+
_xml_namespace_update_references(void *ptr)
50+
{
51+
xmlNsPtr ns = ptr;
52+
if (ns->_private) {
53+
ns->_private = (void *)rb_gc_location((VALUE)ns->_private);
54+
}
55+
}
56+
#else
57+
# define _xml_namespace_update_references 0
58+
#endif
59+
60+
static const rb_data_type_t nokogiri_xml_namespace_type_with_dealloc = {
61+
"Nokogiri/XMLNamespace/WithDealloc",
62+
{0, _xml_namespace_dealloc, 0, _xml_namespace_update_references},
63+
0, 0,
64+
#ifdef RUBY_TYPED_FREE_IMMEDIATELY
65+
RUBY_TYPED_FREE_IMMEDIATELY,
66+
#endif
67+
};
68+
69+
static const rb_data_type_t nokogiri_xml_namespace_type_without_dealloc = {
70+
"Nokogiri/XMLNamespace/WithoutDealloc",
71+
{0, 0, 0, _xml_namespace_update_references},
72+
0, 0,
73+
#ifdef RUBY_TYPED_FREE_IMMEDIATELY
74+
RUBY_TYPED_FREE_IMMEDIATELY,
75+
#endif
76+
};
4577

4678
/*
4779
* call-seq:
@@ -54,7 +86,7 @@ prefix(VALUE self)
5486
{
5587
xmlNsPtr ns;
5688

57-
Data_Get_Struct(self, xmlNs, ns);
89+
Noko_Namespace_Get_Struct(self, xmlNs, ns);
5890
if (!ns->prefix) { return Qnil; }
5991

6092
return NOKOGIRI_STR_NEW2(ns->prefix);
@@ -71,7 +103,7 @@ href(VALUE self)
71103
{
72104
xmlNsPtr ns;
73105

74-
Data_Get_Struct(self, xmlNs, ns);
106+
Noko_Namespace_Get_Struct(self, xmlNs, ns);
75107
if (!ns->href) { return Qnil; }
76108

77109
return NOKOGIRI_STR_NEW2(ns->href);
@@ -87,14 +119,18 @@ noko_xml_namespace_wrap(xmlNsPtr c_namespace, xmlDocPtr c_document)
87119
}
88120

89121
if (c_document) {
90-
rb_namespace = Data_Wrap_Struct(cNokogiriXmlNamespace, 0, 0, c_namespace);
122+
rb_namespace = TypedData_Wrap_Struct(cNokogiriXmlNamespace,
123+
&nokogiri_xml_namespace_type_without_dealloc,
124+
c_namespace);
91125

92126
if (DOC_RUBY_OBJECT_TEST(c_document)) {
93127
rb_iv_set(rb_namespace, "@document", DOC_RUBY_OBJECT(c_document));
94128
rb_ary_push(DOC_NODE_CACHE(c_document), rb_namespace);
95129
}
96130
} else {
97-
rb_namespace = Data_Wrap_Struct(cNokogiriXmlNamespace, 0, dealloc_namespace, c_namespace);
131+
rb_namespace = TypedData_Wrap_Struct(cNokogiriXmlNamespace,
132+
&nokogiri_xml_namespace_type_with_dealloc,
133+
c_namespace);
98134
}
99135

100136
c_namespace->_private = (void *)rb_namespace;

‎ext/nokogiri/xml_node.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1353,7 +1353,7 @@ set_namespace(VALUE self, VALUE namespace)
13531353
Noko_Node_Get_Struct(self, xmlNode, node);
13541354

13551355
if (!NIL_P(namespace)) {
1356-
Data_Get_Struct(namespace, xmlNs, ns);
1356+
Noko_Namespace_Get_Struct(namespace, xmlNs, ns);
13571357
}
13581358

13591359
xmlSetNs(node, ns);

‎test/test_compaction.rb

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,63 @@
33
require "helper"
44

55
describe "compaction" do
6-
it "https://github.com/sparklemotion/nokogiri/pull/2579" do
7-
skip unless GC.respond_to?(:verify_compaction_references)
6+
describe Nokogiri::XML::Node do
7+
it "compacts safely" do # https://github.com/sparklemotion/nokogiri/pull/2579
8+
skip unless GC.respond_to?(:verify_compaction_references)
89

9-
big_doc = "<root>" + ("a".."zz").map { |x| "<#{x}>#{x}</#{x}>" }.join + "</root>"
10-
doc = Nokogiri.XML(big_doc)
10+
big_doc = "<root>" + ("a".."zz").map { |x| "<#{x}>#{x}</#{x}>" }.join + "</root>"
11+
doc = Nokogiri.XML(big_doc)
1112

12-
# ensure a bunch of node objects have been wrapped
13-
doc.root.children.each(&:inspect)
13+
# ensure a bunch of node objects have been wrapped
14+
doc.root.children.each(&:inspect)
1415

15-
# compact the heap and try to get the node wrappers to move
16-
GC.verify_compaction_references(double_heap: true, toward: :empty)
16+
# compact the heap and try to get the node wrappers to move
17+
GC.verify_compaction_references(double_heap: true, toward: :empty)
1718

18-
# access the node wrappers and make sure they didn't move
19-
doc.root.children.each(&:inspect)
19+
# access the node wrappers and make sure they didn't move
20+
doc.root.children.each(&:inspect)
21+
end
22+
end
23+
24+
describe Nokogiri::XML::Namespace do
25+
it "namespace_scopes" do
26+
skip unless GC.respond_to?(:verify_compaction_references)
27+
28+
doc = Nokogiri::XML(<<~EOF)
29+
<root xmlns="http://example.com/root" xmlns:bar="http://example.com/bar">
30+
<first/>
31+
<second xmlns="http://example.com/child"/>
32+
<third xmlns:foo="http://example.com/foo"/>
33+
</root>
34+
EOF
35+
36+
doc.at_xpath("//root:first", "root" => "http://example.com/root").namespace_scopes.inspect
37+
38+
GC.verify_compaction_references(double_heap: true, toward: :empty)
39+
40+
doc.at_xpath("//root:first", "root" => "http://example.com/root").namespace_scopes.inspect
41+
end
42+
43+
it "remove_namespaces!" do
44+
skip unless GC.respond_to?(:verify_compaction_references)
45+
46+
doc = Nokogiri::XML(<<~XML)
47+
<root xmlns:a="http://a.flavorjon.es/" xmlns:b="http://b.flavorjon.es/">
48+
<a:foo>hello from a</a:foo>
49+
<b:foo>hello from b</b:foo>
50+
<container xmlns:c="http://c.flavorjon.es/">
51+
<c:foo c:attr='attr-value'>hello from c</c:foo>
52+
</container>
53+
</root>
54+
XML
55+
56+
namespaces = doc.root.namespaces
57+
namespaces.each(&:inspect)
58+
doc.remove_namespaces!
59+
60+
GC.verify_compaction_references(double_heap: true, toward: :empty)
61+
62+
namespaces.each(&:inspect)
63+
end
2064
end
2165
end

‎test/test_memory_leak.rb

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,47 @@ def test_leaking_namespace_node_strings_with_prefix
191191
end
192192
end
193193

194+
def test_document_remove_namespaces_with_ruby_objects
195+
xml = <<~XML
196+
<root xmlns:a="http://a.flavorjon.es/" xmlns:b="http://b.flavorjon.es/">
197+
<a:foo>hello from a</a:foo>
198+
<b:foo>hello from b</b:foo>
199+
<container xmlns:c="http://c.flavorjon.es/">
200+
<c:foo c:attr='attr-value'>hello from c</c:foo>
201+
</container>
202+
</root>
203+
XML
204+
205+
20.times do
206+
10_000.times do
207+
doc = Nokogiri::XML::Document.parse(xml)
208+
doc.namespaces.each(&:inspect)
209+
doc.remove_namespaces!
210+
end
211+
puts MemInfo.rss
212+
end
213+
end
214+
215+
def test_document_remove_namespaces_without_ruby_objects
216+
xml = <<~XML
217+
<root xmlns:a="http://a.flavorjon.es/" xmlns:b="http://b.flavorjon.es/">
218+
<a:foo>hello from a</a:foo>
219+
<b:foo>hello from b</b:foo>
220+
<container xmlns:c="http://c.flavorjon.es/">
221+
<c:foo c:attr='attr-value'>hello from c</c:foo>
222+
</container>
223+
</root>
224+
XML
225+
226+
20.times do
227+
20_000.times do
228+
doc = Nokogiri::XML::Document.parse(xml)
229+
doc.remove_namespaces!
230+
end
231+
puts MemInfo.rss
232+
end
233+
end
234+
194235
def test_leaking_dtd_nodes_after_internal_subset_removal
195236
# see https://github.com/sparklemotion/nokogiri/issues/1784
196237
100_000.times do |i|

0 commit comments

Comments
 (0)