Ticket #10637 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

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

Reported by: Federico M Pouzols Owned by: Federico M Pouzols
Priority: major Milestone: Release 3.4
Component: Framework Keywords: Maintenance
Cc: Blocked By: #8053, #8484, #8485, #8486, #8487, #8489, #8490, #8491, #8492, #8511
Blocking: Tester: Harry Jeffery

Description (last modified by Federico M Pouzols) (diff)

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).

Change History

comment:1 Changed 6 years ago by Federico M Pouzols

  • Status changed from new to assigned
  • Description modified (diff)

comment:2 Changed 6 years ago by Federico M Pouzols

  • Blocked By 8488 removed

comment:3 Changed 6 years ago by Federico M Pouzols

  • Status changed from assigned to inprogress

comment:4 Changed 6 years ago by Federico M Pouzols

  • Milestone changed from Backlog to Release 3.4

comment:5 Changed 6 years ago by Federico M Pouzols

  • Description modified (diff)

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.

comment:6 Changed 6 years ago by Federico M Pouzols

  • Description modified (diff)

comment:7 Changed 6 years ago by Federico M Pouzols

  • Description modified (diff)

comment:8 Changed 6 years ago by Federico M Pouzols

  • Description modified (diff)

comment:9 Changed 6 years ago by Federico M Pouzols

  • Description modified (diff)

comment:10 Changed 6 years ago by Federico M Pouzols

  • Description modified (diff)

comment:11 Changed 6 years ago by Federico Montesino Pouzols

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

Changeset: 069e211d3735804173ff5c5056bd8314c2969a72

comment:12 Changed 6 years ago by Federico Montesino Pouzols

use AutoPtr - better as code example, re #10637

Changeset: c8b67927e7fcd55f90098ac4cf1104eafafd53ba

comment:13 Changed 6 years ago by Federico Montesino Pouzols

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

Changeset: a04bfffb44665b2d606429ad386f7e1219fb99b4

comment:14 Changed 6 years ago by Federico Montesino Pouzols

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

Changeset: ac75c8d0eaa940aad0db7c425ceb20e443bcec53

comment:15 Changed 6 years ago by Federico Montesino Pouzols

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

Changeset: 1b01f6fb30e895675eae2524c10124018a81d7de

comment:16 Changed 6 years ago by Federico Montesino Pouzols

sort poco headers, kill unused ones, re #10637

Changeset: ef444ab51efe6d4005aacec84b33a8eab60c1f19

comment:17 Changed 6 years ago by Federico Montesino Pouzols

Use AutoPtr, release poco objects, re #10637

Changeset: 41fcf7234623ee50c8bfd5c77fc0cabb880904cf

comment:18 Changed 6 years ago by Federico Montesino Pouzols

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

Changeset: 814bb29840b208c204c9fe29fbdc9af0fe03d744

comment:19 Changed 6 years ago by Federico Montesino Pouzols

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

Changeset: fc3d1c57bf76b70b4ce64533c698844c950f6665

comment:20 Changed 6 years ago by 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

comment:21 Changed 6 years ago by Federico Montesino Pouzols

  • Status changed from inprogress to verify
  • Resolution set to fixed

This is being verified as pull request #486.

comment:22 Changed 6 years ago by Harry Jeffery

  • Status changed from verify to verifying
  • Tester set to Harry Jeffery

comment:23 Changed 6 years ago by Federico Montesino Pouzols

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

Changeset: 50e91dcffc72ceaf202f488ac1697e24a58029cc

comment:24 Changed 6 years ago by Harry Jeffery

Jenkins, retest this please.

comment:25 Changed 6 years ago by Federico Montesino Pouzols

Jenkins, retest this please

comment:26 Changed 6 years ago by Federico Montesino Pouzols

Jenkins, retest this please

comment:27 Changed 6 years ago by Federico Montesino Pouzols

Jenkins, retest this please

comment:28 Changed 6 years ago by Federico Montesino Pouzols

Jenkins, retest this please

comment:29 Changed 6 years ago by 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

comment:30 Changed 6 years ago by Federico Montesino Pouzols

fixed issues with includes after merge, re #10637

Changeset: 8d3f273b064a8b63b6973d0f74f24dbf316fa8a4

comment:31 Changed 6 years ago by Federico Montesino Pouzols

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

comment:32 Changed 6 years ago by Federico Montesino Pouzols

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

Changeset: 555ba6ebf3826bc16d36e211633766d6063c0c35

comment:33 Changed 6 years ago by Federico Montesino Pouzols

fix typo in last commit, re #10637

Changeset: fa3e89cf1c31264c6e7a7de203fecb2d14683335

comment:34 Changed 6 years ago by 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().

comment:35 Changed 6 years ago by Harry Jeffery

  • Status changed from verifying to closed

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

comment:36 Changed 5 years ago by Nick Draper

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

comment:37 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 11479

Note: See TracTickets for help on using tickets.