From e4d587e1e25cb451aaffb6bc9f4f9430cbfb2f84 Mon Sep 17 00:00:00 2001
From: Mike Dalessio <mike.dalessio@gmail.com>
Date: Wed, 31 Dec 2014 01:22:41 -0500
Subject: [PATCH] Ensuring error handling in HTML docs and fragments

Specifically:
- when copying a node between documents, errors are propagated to the
  destination document
- when duplicating a node, silence errors

Fixes #1196
---
 ext/nokogiri/xml_node.c             | 16 +++++++++++++++-
 ext/nokogiri/xml_syntax_error.c     |  4 ++++
 ext/nokogiri/xml_syntax_error.h     |  1 +
 test/html/test_document.rb          | 14 ++++++++++++++
 test/html/test_document_fragment.rb | 23 +++++++++++++++++++++++
 5 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/ext/nokogiri/xml_node.c b/ext/nokogiri/xml_node.c
index e82108af27..7043f5db22 100644
--- a/ext/nokogiri/xml_node.c
+++ b/ext/nokogiri/xml_node.c
@@ -234,7 +234,15 @@ static VALUE reparent_node_with(VALUE pivot_obj, VALUE reparentee_obj, pivot_rep
      *  reparent the actual reparentee, so we reparent a duplicate.
      */
     nokogiri_root_node(reparentee);
-    if (!(reparentee = xmlDocCopyNode(reparentee, pivot->doc, 1))) {
+
+    xmlResetLastError();
+    xmlSetStructuredErrorFunc((void *)rb_iv_get(DOC_RUBY_OBJECT(pivot->doc), "@errors"), Nokogiri_error_array_pusher);
+
+    reparentee = xmlDocCopyNode(reparentee, pivot->doc, 1) ;
+
+    xmlSetStructuredErrorFunc(NULL, NULL);
+
+    if (! reparentee) {
       rb_raise(rb_eRuntimeError, "Could not reparent node (xmlDocCopyNode)");
     }
   }
@@ -473,7 +481,13 @@ static VALUE duplicate_node(int argc, VALUE *argv, VALUE self)
 
   Data_Get_Struct(self, xmlNode, node);
 
+  xmlResetLastError();
+  xmlSetStructuredErrorFunc(NULL, Nokogiri_error_silencer);
+
   dup = xmlDocCopyNode(node, node->doc, (int)NUM2INT(level));
+
+  xmlSetStructuredErrorFunc(NULL, NULL);
+
   if(dup == NULL) return Qnil;
 
   nokogiri_root_node(dup);
diff --git a/ext/nokogiri/xml_syntax_error.c b/ext/nokogiri/xml_syntax_error.c
index 0b240f05a5..b36f667e03 100644
--- a/ext/nokogiri/xml_syntax_error.c
+++ b/ext/nokogiri/xml_syntax_error.c
@@ -7,6 +7,10 @@ void Nokogiri_error_array_pusher(void * ctx, xmlErrorPtr error)
   rb_ary_push(list,  Nokogiri_wrap_xml_syntax_error(error));
 }
 
+void Nokogiri_error_silencer(void * ctx, xmlErrorPtr error)
+{
+}
+
 void Nokogiri_error_raise(void * ctx, xmlErrorPtr error)
 {
   rb_exc_raise(Nokogiri_wrap_xml_syntax_error(error));
diff --git a/ext/nokogiri/xml_syntax_error.h b/ext/nokogiri/xml_syntax_error.h
index 58475cb852..5a6e1172ba 100644
--- a/ext/nokogiri/xml_syntax_error.h
+++ b/ext/nokogiri/xml_syntax_error.h
@@ -6,6 +6,7 @@
 void init_xml_syntax_error();
 VALUE Nokogiri_wrap_xml_syntax_error(xmlErrorPtr error);
 void Nokogiri_error_array_pusher(void * ctx, xmlErrorPtr error);
+void Nokogiri_error_silencer(void * ctx, xmlErrorPtr error);
 NORETURN(void Nokogiri_error_raise(void * ctx, xmlErrorPtr error));
 
 extern VALUE cNokogiriXmlSyntaxError;
diff --git a/test/html/test_document.rb b/test/html/test_document.rb
index 8b4dae0c6e..30777c6c6c 100644
--- a/test/html/test_document.rb
+++ b/test/html/test_document.rb
@@ -621,6 +621,20 @@ def test_capturing_nonparse_errors_during_document_clone
         copy = original.dup
         assert_equal original_errors, copy.errors
       end
+
+      def test_capturing_nonparse_errors_during_node_copy_between_docs
+        doc1 = Nokogiri::HTML("<div id='unique'>one</div>")
+        doc2 = Nokogiri::HTML("<div id='unique'>two</div>")
+        node1 = doc1.at_css("#unique")
+        node2 = doc2.at_css("#unique")
+
+        original_errors = doc1.errors.dup
+
+        node1.add_child node2
+
+        assert_equal original_errors.length+1, doc1.errors.length
+        assert_match /ID unique already defined/, doc1.errors.last.to_s
+      end
     end
   end
 end
diff --git a/test/html/test_document_fragment.rb b/test/html/test_document_fragment.rb
index 5b9a154a15..fa7fd0f601 100644
--- a/test/html/test_document_fragment.rb
+++ b/test/html/test_document_fragment.rb
@@ -267,6 +267,29 @@ def test_error_propagation_on_fragment_parse_in_node_context_should_not_include_
         assert frag.errors.any?{|err| err.to_s =~ /Tag hello invalid/}, "errors should be on the context node's document"
         assert frag.errors.none?{|err| err.to_s =~ /jimmy/}, "errors should not include pre-existing document errors"
       end
+
+      def test_capturing_nonparse_errors_during_fragment_clone
+        # see https://github.com/sparklemotion/nokogiri/issues/1196 for background
+        original = Nokogiri::HTML.fragment("<div id='unique'></div>")
+        original_errors = original.errors.dup
+
+        copy = original.dup
+        assert_equal original_errors, copy.errors
+      end
+
+      def test_capturing_nonparse_errors_during_node_copy_between_fragments
+        frag1 = Nokogiri::HTML.fragment("<div id='unique'>one</div>")
+        frag2 = Nokogiri::HTML.fragment("<div id='unique'>two</div>")
+        node1 = frag1.at_css("#unique")
+        node2 = frag2.at_css("#unique")
+
+        original_errors = frag1.errors.dup
+
+        node1.add_child node2 # we should also not see an error on stderr
+
+        assert_equal original_errors.length+1, frag1.errors.length
+        assert_match /ID unique already defined/, frag1.errors.last.to_s
+      end
     end
   end
 end