[bug] XML::Builder blocks don't restore the parent node when an error is raised from the block

[ric2b]

When using the XML builder, exceptions that escape a node's block will cause that node to become the parent of sibling nodes.

For example, this:

builder = Nokogiri::XML::Builder.new do |xml|
  xml.my_root do
    begin
      xml.a do
        xml.a1
        xml.a2
        raise StandardError.new
      end
    rescue StandardError
    end

    xml.b
  end
end

puts builder.to_xml

Outputs this (bshould not be a child of a):

<?xml version="1.0"?>
<my_root>
  <a>
    <a1/>
    <a2/>
    <b/>
  </a>
</my_root>

Expected behavior

b should be a sibling of a:

<?xml version="1.0"?>
<my_root>
  <a>
    <a1/>
    <a2/>
  </a>
  <b/>
</my_root>

Environment

# Nokogiri (1.12.5)
    ---
    warnings: []
    nokogiri:
      version: 1.12.5
      cppflags:
      - "-I/Users/<username>/.rbenv/versions/3.0.2/lib/ruby/gems/3.0.0/gems/nokogiri-1.12.5-x86_64-darwin/ext/nokogiri"
      - "-I/Users/<username>/.rbenv/versions/3.0.2/lib/ruby/gems/3.0.0/gems/nokogiri-1.12.5-x86_64-darwin/ext/nokogiri/include"
      - "-I/Users/<username>/.rbenv/versions/3.0.2/lib/ruby/gems/3.0.0/gems/nokogiri-1.12.5-x86_64-darwin/ext/nokogiri/include/libxml2"
      ldflags: []
    ruby:
      version: 3.0.2
      platform: x86_64-darwin20
      gem_platform: x86_64-darwin-20
      description: ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin20]
      engine: ruby
    libxml:
      source: packaged
      precompiled: true
      patches:
      - 0001-Remove-script-macro-support.patch
      - 0002-Update-entities-to-remove-handling-of-ssi.patch
      - 0003-libxml2.la-is-in-top_builddir.patch
      - 0004-use-glibc-strlen.patch
      - 0005-avoid-isnan-isinf.patch
      - 0006-update-automake-files-for-arm64.patch
      - 0007-Fix-XPath-recursion-limit.patch
      libxml2_path: "/Users/<username>/.rbenv/versions/3.0.2/lib/ruby/gems/3.0.0/gems/nokogiri-1.12.5-x86_64-darwin/ext/nokogiri"
      memory_management: ruby
      iconv_enabled: true
      compiled: 2.9.12
      loaded: 2.9.12
    libxslt:
      source: packaged
      precompiled: true
      patches:
      - 0001-update-automake-files-for-arm64.patch
      - 0002-Fix-xml2-config-check-in-configure-script.patch
      datetime_enabled: true
      compiled: 1.1.34
      loaded: 1.1.34
    other_libraries:
      zlib: 1.2.11
      libiconv: '1.15'
      libgumbo: 1.0.0-nokogiri

Additional context

I'm pretty sure the issue is here: https://github.com/sparklemotion/nokogiri/blob/v1.12.5/lib/nokogiri/xml/builder.rb#L422-L438

@parent = old_parent should happen inside an ensure so that it happens even in the event of a raise escaping the block. We would be happy to open a PR with the fix and a few tests if you're interested.

[flavorjones]

@ric2b Thanks for opening this issue! That's certainly odd behavior, and I agree we should change it. I'll try to set aside some time this weekend to dig in a bit.

[flavorjones]

@ric2b Just saw your offer to ship a PR. That would be lovely! I agree with your diagnosis that we need an ensure to restore the parent context.

[flavorjones]

Thanks for closing this!