Close and review all leak issues with Poco::XML document, nodelist, etc. objects

[Federico M Pouzols]

These issues include:

• leaks / unreleased objects
• objects released via the release() method instead of the (preferred) AutoPtr

This is an umbrella ticket for a bunch of tickets (see 'Blocked By' list) and possibly a few more issues. Work on this seems to have started in #8053 which spawned a good bunch of tickets. Some relatively old tickets about this were closed but some others remain open. Note that some of the issues may have been fixed since then, but definitely not all of them.

To do: make sure that there are no leaks with document and nodelist objects, and that these are not released explicitly with the release() method but but using AutoPtr. In some cases, release() might be fine: ​http://trac.mantidproject.org/mantid/ticket/8487#comment:26.

Relevant calls that need to be checked include at least:

• parseString
• getElementsByTagName
• createTextNode
• create*Node

• Poco::XML::Node::attributes()
• Poco::XML::Node::childNodes()

There could be additional issues, like in FilterChannelTest.

This ticket should help reducing the list of valgrind memory issues, although it may not be easy to see this

Addition (see #comment:5 for details): check that the code using Poco::XML does not include too many unnecessary Poco headers, and that Poco XML classes are forward-declared as much as possible in Mantid headers.

Addition: also watch out for calls to Poco::XML::Node::attributes() and - Poco::XML::Node::childNodes(), the returned map / list must be released (​http://pocoproject.org/docs/Poco.XML.Node.html#28889).

[Federico M Pouzols]

As part of this ticket the following issue could also be dealt with: when using Poco::XML, the current code sometimes includes too many headers that are not needed. This concerns Poco::XML related headers and also for example Poco/File.h which is included sometimes even if that functionality is not used, probably as a leftover from old code, or a "copy-paste" side effect. Also in several header files, includes could be replaced by forward declarations of Poco::Document, etc. classes. This has been partially improved in some of the tickets that block this one.

[Federico Montesino Pouzols]

use AutoPtr - better code example, kill unnec Poco headers, re #10637

Changeset: 069e211d3735804173ff5c5056bd8314c2969a72

[Federico Montesino Pouzols]

use AutoPtr - better as code example, re #10637

Changeset: c8b67927e7fcd55f90098ac4cf1104eafafd53ba

[Federico Montesino Pouzols]

Use Poco::AutoPtr, rearrange lib Poco headers, re #10637

Changeset: a04bfffb44665b2d606429ad386f7e1219fb99b4

[Federico Montesino Pouzols]

use AutoPtr, sort Poco headers, uncomment+disable tests, re #10637

Changeset: ac75c8d0eaa940aad0db7c425ceb20e443bcec53

[Federico Montesino Pouzols]

rearrange poco headers and un-include unused ones, re #10637

Changeset: 1b01f6fb30e895675eae2524c10124018a81d7de

[Federico Montesino Pouzols]

sort poco headers, kill unused ones, re #10637

Changeset: ef444ab51efe6d4005aacec84b33a8eab60c1f19

[Federico Montesino Pouzols]

Use AutoPtr, release poco objects, re #10637

Changeset: 41fcf7234623ee50c8bfd5c77fc0cabb880904cf

[Federico Montesino Pouzols]

sort poco headers and remove/un-include unused ones, re #10637

Changeset: 814bb29840b208c204c9fe29fbdc9af0fe03d744

[Federico Montesino Pouzols]

sort lib Poco headers, remove/un-include unused ones, re #10637

Changeset: fc3d1c57bf76b70b4ce64533c698844c950f6665

[Federico Montesino Pouzols]

Merge remote-tracking branch 'origin/master' into 10637_close_review_all_leak_issues_with_poco_xml

Conflicts:

Code/Mantid/Framework/Kernel/src/InstrumentInfo.cpp

Sort out conflict in includes section, re #10637

Changeset: b362ec863907b43120e83dc35480de970cf260c1

[Federico Montesino Pouzols]

This is being verified as ​pull request #486.

[Federico Montesino Pouzols]

check pointers before ->release() in destructor, re #10637

Changeset: 50e91dcffc72ceaf202f488ac1697e24a58029cc

[Harry Jeffery]

Jenkins, retest this please.

[Federico Montesino Pouzols]

Jenkins, retest this please

[Federico Montesino Pouzols]

Jenkins, retest this please

[Federico Montesino Pouzols]

Jenkins, retest this please

[Federico Montesino Pouzols]

Jenkins, retest this please

[Federico Montesino Pouzols]

Merge remote-tracking branch 'origin/master' into 10637_close_review_all_leak_issues_with_poco_xml

Conflicts:

Code/Mantid/Framework/DataObjects/src/CoordTransformAffine.cpp Code/Mantid/Framework/DataObjects/src/CoordTransformDistance.cpp Code/Mantid/Framework/DataObjects/test/CoordTransformAffineParserTest.h Code/Mantid/Framework/DataObjects/test/CoordTransformDistanceParserTest.h Code/Mantid/Framework/SINQ/src/SINQHMListener.cpp

Sorting out conflict with big MDEvents move, re #10637

Changeset: 01941117d9a2600664be01ba37e434d626e6a0d9

[Federico Montesino Pouzols]

fixed issues with includes after merge, re #10637

Changeset: 8d3f273b064a8b63b6973d0f74f24dbf316fa8a4

[Federico Montesino Pouzols]

I've fixed the merge conflict. Let's see if the issue with LoadMaskTest on mac persists.

[Federico Montesino Pouzols]

don't release m_pRootElem, causes double free with m_pDoc, re #10637

Changeset: 555ba6ebf3826bc16d36e211633766d6063c0c35

[Federico Montesino Pouzols]

fix typo in last commit, re #10637

Changeset: fa3e89cf1c31264c6e7a7de203fecb2d14683335

[Federico Montesino Pouzols]

Aha, something was causing a segfault in the LoadMask unit test, only on mac, and only in say 30-50% of builds. But I got one crash where the error message was clear: double free corruption. I realized that I added a couple of ->release() calls in the LoadMask destructor, one for m_pDoc and another one for m_pRootElem. That's wrong, m_pRootElem is assigned from Poco::DOM::Document::documentElement() which does not require a release and is simply a pointer to the root element of the document tree that is freed when doing m_pDoc->release().

[Harry Jeffery]

Merge pull request #486 from mantidproject/10637_close_review_all_leak_issues_with_poco_xml

Close and review all leak issues with Poco::XML document, nodelist, etc. objects

Full changeset: ​051c23a6a51d68659f13126e34141fbdc17ea10b

[Nick Draper]

Somehow these slipped through without a resolution. Set to Fixed.

[Stuart Campbell]

This ticket has been transferred to github issue ​11479