Memory leak with ticpp::Element::GetDocument()

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Load an xml (will most likely work just creating a document)
2. Get an element from the XML
3. Call GetDocument() on the element

What is the expected output? What do you see instead?
Should get a pointer to the document which you then delete, which will 
decrement the ref count

What version of the product are you using? On what operating system?
svn trunk, windows 7

Please provide any additional information below.
When you call GetDocument on an Element, it eventually (Node::GetDocument()) 
calls this code:

Document* temp = new Document( doc );
doc->m_spawnedWrappers.push_back( temp );
return temp;

The pointer that gets returned from this function is impossible to destroy 
cleanly (as far as I'm aware).

Either, you delete the pointer returned from GetDocument(), in which case, you 
get a double-free error when TiCppRC::DeleteSpawnedWrappers() is called when 
the last reference to the document is destroyed. Or you do not delete it, which 
leaves the document with a reference count of 1 and lots of nice memory leaks 
due to the document not being destroyed.

Original issue reported on code.google.com by [email protected] on 20 Nov 2010 at 4:53

[GoogleCodeExporter]

The only workaround I've found is to re-assign to the pointer received from 
GetDocument, i.e.

ticpp::Document* doc = node->GetDocument();

... subsequently ...

doc = ticpp::Document();

Original comment by [email protected] on 2 Jun 2014 at 8:47

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Need to amend previous comment. Instead of:

   doc = ticpp::Document();

needs to be:

   *doc = ticpp::Document();

Sorry for the typo...

Original comment by [email protected] on 3 Jun 2014 at 3:29

[GoogleCodeExporter]

OK, one more note and I'll leave it alone.  Here's what I'm using as a work 
around:

class DocumentPtr
{
public:
  DocumentPtr(ticpp::Node const * node) : document(node->GetDocument()) {}
  ~DocumentPtr() { *document = ticpp::Document(); }
  ticpp::Document* operator->() { return document; }
  ticpp::Document& operator*() { return *document; }
  operator ticpp::Document&() { return *document; }
  operator ticpp::Document*() { return document; }

private:
  DocumentPtr(DocumentPtr const&);              // No copy construct
  DocumentPtr& operator=(DocumentPtr const &);  // No copy assignment
  ticpp::Document* document;
};

I haven't beat on it a bunch but it works OK in this context:
   ticpp::Element const * e = findElement(DocumentPtr(refElem), path);
where
   ticpp::Element const * e = findElement(refElem->GetDocument(), path);
didn't. 

FWIW the signature for findElement is:
   ticpp::Element* findElement(ticpp::Node const *, std::string const&)

Original comment by [email protected] on 3 Jun 2014 at 4:46

[davidhesselbom]

I found the same issue with the Parent function when it returns a Document, and I suspect it has the same root cause.

    ticpp::Document document;
    document.Parse(xmlString);

    // Pick one of the next 2 lines, not both
    auto node = document.FirstChild();
    auto node = document.FirstChild()->FirstChild();
    auto parent = node->Parent(); // parent is now either a Document or an Element 

In the case where parent is a Document, I get a memory leak, yet calling delete parent causes a double-free error. I also don't know of a "clean" way to know, before calling Parent, whether it will return a Document or not, so that the return of a Document can be avoided in the first place.

Re-assigning the pointer with

*parent = ticpp::Document()

as described above does not fix the issue for me; the leak remains.

What does fix the issue is

parent->~Node();

but, as anyone will tell me, explicitly calling a destructor is a bad idea. So am I using the library wrong, or is there a bug?

[davidhesselbom]

Also, I can confirm the following piece of code also causes a leak, as described by the OP:

ticpp::Document document;
document.Parse(xmlString);

auto document2 = document.GetDocument();

and that, as above,

document2->~Node();

fixes the leak, whereas

delete document2;

causes a double-free.

In this case, where document2 has been assigned to document.GetDocument() rather than document.FirstChild()->Parent(),

*document2 = ticpp::Document();

does fix the memory leak, as suggested by OP, unlike in my previous example, where reassigning parent did not.

[davidhesselbom]

Good news: adding m_impRC->InitRef(); to the body of the constructor Document::Document( TiXmlDocument* document ) seems to fix the problem, both when calling GetDocument(), as OP did, and when Parent returns a Document, as in my example.

In both cases, neither delete or calling the destructor is necessary before the variable goes out of scope.

I don't know whether this change has any other, unwanted effects, or why the code doesn't do this already. The m_impRC->InitRef(); call is missing from Element::Element( TiXmlElement* element ) and Comment::Comment( TiXmlComment* comment ), too, even though it is present in their respective other constructors.

I'll make a PR and hopefully get feedback of some kind...