Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug] Document Fragment #clone does not behave like #dup with respect to the owning document #2908

Open
flavorjones opened this issue Jun 20, 2023 · 3 comments
Milestone

Comments

@flavorjones
Copy link
Member

Please describe the bug

While working on upgrading Action Text to HTML5, some tests were failing that I traced back to this repro:

class Minitest::Spec
  describe "HTML4" do
    let(:document) { Nokogiri::HTML4::Document.parse("<div>doc</div>") }
    let(:fragment) { document.fragment("<div>frag</div>") }

    it "#dup makes a copy of the fragment with a new document" do
      dup = fragment.dup
      refute_nil(dup.document)
      refute_same(dup.document, fragment.document)
    end

    it "#clone makes a copy of the fragment with a new document" do
      clone = fragment.clone
      refute_nil(clone.document)
      refute_same(clone.document, fragment.document) ######## this fails ########
    end
  end

  describe "HTML5" do
    let(:document) { Nokogiri::HTML5::Document.parse("<div>doc</div>") }
    let(:fragment) { document.fragment("<div>frag</div>") }

    it "#dup makes a copy of the fragment with a new document" do
      dup = fragment.dup
      refute_nil(dup.document)
      refute_same(dup.document, fragment.document)
    end

    it "#clone makes a copy of the fragment with a new document" do
      clone = fragment.clone
      refute_nil(clone.document) ######## this fails ########
      refute_same(clone.document, fragment.document)
    end
  end
end

fails with

  1) Failure:
HTML4#test_0002_#clone makes a copy of the fragment with a new document [../rails/nokogiri-clone-bug.rb:54]:
Expected #<Nokogiri::HTML4::Document:0x67c name="document" children=[#<Nokogiri::XML::DTD:0x618 name="html">, #<Nokogiri::XML::Element:0x668 name="html" children=[#<Nokogiri::XML::Element:0x654 name="body" children=[#<Nokogiri::XML::Element:0x640 name="div" children=[#<Nokogiri::XML::Text:0x62c "doc">]>]>]>]> (oid=1660) to not be the same as #<Nokogiri::HTML4::Document:0x67c name="document" children=[#<Nokogiri::XML::DTD:0x618 name="html">, #<Nokogiri::XML::Element:0x668 name="html" children=[#<Nokogiri::XML::Element:0x654 name="body" children=[#<Nokogiri::XML::Element:0x640 name="div" children=[#<Nokogiri::XML::Text:0x62c "doc">]>]>]>]> (oid=1660).

  2) Failure:
HTML5#test_0002_#clone makes a copy of the fragment with a new document [../rails/nokogiri-clone-bug.rb:70]:
Expected nil to not be nil.

Specifically:

  • HTML4::DocumentFragment#clone re-uses the original's Document
  • HTML5::DocumentFragment#clone does not set a document at all!

Expected behavior

The desired behavior matches #dup for node and node-like things: to make a copy of the parent document to avoid accumulating nodes that cannot be freed until the parent document is freed (see #1063 and #1834).

Please also note, while we're in here, that there are expected differences between #dup and #clone that we're also not honoring, see #316 for a description of those problems (essentially, the singleton class). If we tackle this issue, we should tackle that one as well.

@flavorjones flavorjones added the state/needs-triage Inbox for non-installation-related bug reports or help requests label Jun 20, 2023
@flavorjones flavorjones removed the state/needs-triage Inbox for non-installation-related bug reports or help requests label Jun 28, 2023
@flavorjones flavorjones added this to the v1.17.0 milestone Jun 28, 2024
@sharvy
Copy link
Contributor

sharvy commented Aug 1, 2024

@flavorjones I don't see help needed label on this issue, does that mean you (or someone else) are already working on it? If no one has started working on it yet, do you think I should give it a try?

@flavorjones
Copy link
Member Author

@sharvy If you'd like to give it a try, please do! I solved #316 mentioned above already so ignore that bit. If you get stuck or see something weird, ping me and I'm happy to help!!

@sharvy
Copy link
Contributor

sharvy commented Aug 1, 2024

@flavorjones Thanks, I'm on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants