memory leak

[flavorjones]

Reproduced under:

# Nokogiri (1.5.6)
    ---
    warnings: []
    nokogiri: 1.5.6
    ruby:
      version: 1.9.3
      platform: x86_64-linux
      description: ruby 1.9.3p327 (2012-11-10 revision 37606) [x86_64-linux]
      engine: ruby
    libxml:
      binding: extension
      compiled: 2.8.0
      loaded: 2.8.0

Here's the repro script from @armhold:

#!/usr/bin/env ruby

require 'nokogiri'

def get_memory_usage
  `ps -o rss= -p #{Process.pid}`.to_i
end

HTML_CONTENT = "This is some html<br/>This is some more html<br/>" * 10

def run_nokogiri
  doc = Nokogiri::HTML.parse(HTML_CONTENT)
  doc.css("br").each { |node| node.replace("\n ")}
  doc.text

  nil
end

i = 0
loop do
  run_nokogiri

  i += 1
  if i % 1000 == 0
    GC.start

    puts "iteration #{i}"
    puts "current memory: #{get_memory_usage}"
  end
end

[flavorjones]

Yeargh, this causes a segfault on 1.8.7. Reverting for now, I'll revisit when I have a bit more time to dig in.

[ender672]

The leak can be whittled down to:

loop do
  doc = Nokogiri::XML.parse('<a/>')
  doc.root.send(:in_context, '<b/>', Nokogiri::XML::ParseOptions::DEFAULT_XML)
end

[flavorjones]

@ender672 take a look at my reverted commit above. It crashed on 1.8.7 but was fine (even under Valgrind) on 1.9.3 and 2.0.0. The issue is partly that the fragment nodes, if left unparented, are not GCed as the document doesn't know they exist.

We've got to figure out a way to make sure top-level fragment nodes are known by the document; I thought my patch addressed it, but did not have time to investigate why 1.8.7 segfaulted (double-free).

Can you investigate?

[ender672]

Your solution looks correct, but it exposes a subtle nuance in nokogiri_root_node() and libxml2's xmlAddChild() function.

When Nokogiri frees a document, it goes through the list of nodes marked by nokogiri_root_node() and adds them to the document root via xmlAddChild() so that xmlFreeDoc() takes care of them later. In ext/nokogiri/xml_document.c:13

xmlAddChild((xmlNodePtr)doc, node);

Interestingly, xmlAddChild() will not touch the next field in node. It pretty much assumes that this has been set to NULL before being added.

When nokogiri creates the new nodes in in_context(), the new nodes are linked via the next field. When nokogiri later frees the document, the order in which xmlAddChild() is called can end up with a situation where the last node's next field is broken. This order is ruby implementation specific and explains why 1.8.7 has this problem and 1.9.3 doesn't.

I think the best solution is to set the next field to NULL before calling xmlAddChild(). For example, in ext/nokogiri/xml_document.c:13:

node->next = NULL;
xmlAddChild((xmlNodePtr)doc, node);

However, I'm a little nervous about doing this in an rc release. There is a possibility that nokogiri is taking advantage of this quirk and that setting node->next to NULL will expose a new memory leak.

[ender672]

After taking some time to look at the users of nokogiri_root_node(), I feel a lot more comfortable with this fix.

In all of Nokogiri's unit tests, node->next is always NULL, with the exception of nodes added via in_context().

I feel we can safely add this to the rc. Any concerns?

[ender672]

The memory leak test that have infinite loops eventually should be moved out of the unit test framework. In the meantime, you can test the one related to this ticket with:

$ NOKOGIRI_GC=true bundle exec ruby -Ilib:test test/test_memory_leak.rb -n '/parser_leak_ii/'

[flavorjones]

Sorry for not responding to your earlier comments. This looks good! I'm going to cut another bugfix release today (for this and other issues).