Cast LibXML::Doc* to LibXML::Node*

[toddsundsted]

LibXML.xmlFreeNode(node) on line 60 is never called because node.value.doc is a LibXML::Doc* not a LibXML::Node* and so the comparison always evaluates to false. The equality method in Comparable only kicks in if the receiver and the one parameter are the same type. This PR casts doc to the correct type of the comparison.

The following spec demonstrates the problem. If you adjust the number of iterations, the memory allocated scales linearly with iterations and never plateaus. Unlinking nodes by iterating over children and xpath nodes both demonstrate the problem.

require "spec"
require "xml"

describe "XML memory management" do
  it "does not leak parser memory" do
    xml = %Q|<root>#{"<child/>" * 10}</root>|

    GC.collect
    baseline_rss = get_rss_kb

    1_000_000.times do
      doc = XML.parse(xml)
      root = doc.root.not_nil!
      root.children.each(&.unlink)
    end

    # 1_000_000.times do
    #   doc = XML.parse(xml)
    #   doc.xpath_nodes("//child").each(&.unlink)
    # end

    GC.collect
    final_rss = get_rss_kb
    growth = final_rss - baseline_rss

    p growth: growth

    (growth < 50_000).should be_true
  end
end

private def get_rss_kb : Int64
  {% if flag?(:darwin) %}
    `ps -o rss= -p #{Process.pid}`.strip.to_i64
  {% else %}
    File.read("/proc/self/status").lines.each do |line|
      return line.split[1].to_i64 if line.starts_with?("VmRSS:")
    end
    0_i64
  {% end %}
end

[straight-shoota]

Both your PRs are reusing the same master branch on your fork, which killed #16406.
It's better to create dedicated branches for each change 😅

In order to avoid further disruptions, we should keep this PR as is and reopen a new PR with the changes from #16406 (36c769a) on a different branch.

[ysbaddaden]

Urgh, I wish a linter would notice "always true" and "always false" comparisons and warn about them.

Thank you @toddsundsted 🙇

[ysbaddaden]

I could reopen #16406 from the github pr refs. See #16414.

[straight-shoota]

This is an easy trap. I'm wondering if Pointer#== should always compare the address, regardless of the pointer type. They're partially identical because they point to the same start address. The exact memory size may differ between types, though. And interpretation differs anyway. 🤔
But it's not about the objects themselves, it's about pointers to them. And pointers are primarily defined by their address.

[toddsundsted]

Both your PRs are reusing the same master branch on your fork, which killed #16406. It's better to create dedicated branches for each change 😅

Sorry! Got careless.

[toddsundsted]

ugh again :-( this is the last time, I swear!