[Fix] Remaining memory leaks in tests

[dvdvgt]

Description

I had a look at the remaining memory leaks in the tests. Sadly almost all have them are due to leaks in third party dependencies and therefore hardly fixable.

Name	Memory Leak source	Description
DetectabilitySimulation_test	libsvm	Unresolvable
FeatureFinderAlgorithmPicked_test	Raw pointer	Fixed by using a std::shared_ptr. Not sure if this introduces a relevant performance penalty. Thoughts?
FeatureFinderMetabo_test	libsvm	Unresolvable
LibSVMEncoder_test	libsvm	Unresolvable
MzMLFile_test	xercesc_3_2::MemoryManagerImpl::allocate(unsigned long)	Unresolvable
MzMLSpectrumDecoder_test	xercesc_3_2::MemoryManagerImpl::allocate(unsigned long), ...	Unresolvable
OfflinePrecursorIonSelection_test	libsvm	Unresolvable
OSWFile_test	libsqlite	Unresolvable
PepXMLFile_test	probably a false positiv	Allocation here and deletion here. Should be fine.
PrecursorIonSelectionPreprocessing_test	libsvm	Unresolvable
PSLPFormulation_test	libsvm	Unresolvable
SimpleSVM_test	libsvm	Unresolvable
SVMWrapper_test	libsvm	Unresolvable
TheoreticalSpectrumGenerator_test	probably a false positiv	Allocation here and deletion here. Should be fine.
TOPP_ExecutePipeline_1	Qt	Unresolvable
TOPP_FeatureFinderCentroided_1	Raw pointer in FeatureFinderAlgorithmPicked	Fixed. See prior entry FeatureFinderAlgorithmPicked_test
TOPP_PTModel_1	libsvm	Unresolvable
TOPP_RTModel_1	libsvm	Unresolvable
TOPP_RTModel_2	libsvm	Unresolvable
TOPP_RTModel_3	libsvm	Unresolvable
TOPP_RTModel_4	libsvm	Unresolvable
TOPPView_test	Qt	Unresolvable
UnimodXMLFile_test	xercesc_3_2::DefaultHandler	Unresolvable

Checklist:

• Make sure that you are listed in the AUTHORS file
• Add relevant changes and new features to the CHANGELOG file
• I have commented my code, particularly in hard-to-understand areas
• New and existing unit tests pass locally with my changes
• Updated or added python bindings for changed or new classes. (Tick if no updates were necessary.)

How can I get additional information on failed tests during CI:

If your PR is failing you can check out

• http://cdash.openms.de/index.php?project=OpenMS and look for your PR. If you click in the column that lists the failed tests you will get detailed error messages.

Note:

• Once you opened a PR try to minimize the number of pushes to it as every push will trigger CI (automated builds and test) and is rather heavy on our infrastructure (e.g., if several pushes per day are performed).

[jpfeuffer]

What does this do in case of actual deletion during assignment? Does not look right but I only gave it a quick thought.

    analysis_results_ = nullptr;
    if (source.analysis_results_ != nullptr)
    {
      // free memory first
      delete analysis_results_;

[jpfeuffer]

In the TSG_test, is add_metainfo_ enabled? The move here looks suspicious, especially for a class that inherits from both std::vector and MetaInfoDescription with a default move ctor.

OpenMS/src/openms/source/CHEMISTRY/TheoreticalSpectrumGenerator.cpp

Line 207 in bf46a11

spectrum.getIntegerDataArrays().push_back(std::move(*charges));

?

I am also not sure if it is safe (or catchable by ASAN), if a moved_from pointer is deleted. Maybe make it a nullptr after the move?

[jpfeuffer]

While I think you are correct with the rest of the leaks to be related to third-party libraries, I also think that probably some of them could be due to misuse of those libraries and could still be solved on our end.
If this can be done with a reasonable effort, is another question.

Thanks for investigating.

[dvdvgt]

What does this do in case of actual deletion during assignment? Does not look right but I only gave it a quick thought.

OpenMS/src/openms/source/METADATA/PeptideHit.cpp

Lines 129 to 135 in bf46a11

	analysis_results_ = nullptr;
	if (source.analysis_results_ != nullptr)
	{
	// free memory first
	delete analysis_results_;
	analysis_results_ = new std::vector<PepXMLAnalysisResult>(*source.analysis_results_);
	}

Yes, it does look a bit weird. analysis_results_ is set to a nullptr which means if analysis_results_ is deleted it is always a nullptr that gets deleted. A quick search revealed that deleting a nullptr is safe. However the memory analysis_results_ points to before being assigned the nullptr is never freed. I'd change that so that analysis_results_ is always deleted. What you think?

---

In the TSG_test, is add_metainfo_ enabled? The move here looks suspicious, especially for a class that inherits from both std::vector and MetaInfoDescription with a default move ctor.

Form some tests it is activated for others it is not.

OpenMS/src/tests/class_tests/openms/source/TheoreticalSpectrumGenerator_test.cpp

Lines 224 to 227 in 34f271d

	param.setValue("add_metainfo", "false");
	param.setValue("add_losses", "true");
	ptr->setParameters(param);
	ptr->getSpectrum(spec, peptide, 1, 1);

OpenMS/src/tests/class_tests/openms/source/TheoreticalSpectrumGenerator_test.cpp

Lines 251 to 254 in 34f271d

	param.setValue("add_metainfo", "true");
	param.setValue("add_losses", "true");
	ptr->setParameters(param);
	ptr->getSpectrum(spec, peptide, 1, 1);

Maybe make it a nullptr after the move?

I'll do that.

---

If this can be done with a reasonable effort, is another question.

I figured that too. Fixing some of these memory leaks would require some major refactoring. That's probably not worth it considering only some bytes are leaking.

[jpfeuffer]

Yep, give those a try. Do you have an ASAN setup on your hardware?

[dvdvgt]

Regarding TheoreticalSpectrumGenerator_test I noticed that the leaks only occur here:

OpenMS/src/tests/class_tests/openms/source/TheoreticalSpectrumGenerator_test.cpp

Line 598 in 34f271d

TEST_EXCEPTION(Exception::InvalidSize, t_gen.getSpectrum(tmp, tmp_aa, 1, 1));

OpenMS/src/tests/class_tests/openms/source/TheoreticalSpectrumGenerator_test.cpp

Line 604 in 34f271d

TEST_EXCEPTION(Exception::InvalidSize, t_gen.getSpectrum(tmp, tmp_aa, 1, 1));

So I concluded that getSpectrum is generally safe except for a premature function exit like an exception. If an exception occurs the delete statement might not be reached and the memory is not freed. The only way I know how to prevent that is to use smart pointers because they'll clean up after you in any case. In this case std::shared_ptr because ion_names and charges need to have shared access since they can be passed to more than one function like addPeaks_, addPrecursorPeaks_, ....
As always std::shared_ptr introduces some overhead but I think it should be negligible in this case. What do you think?

---

Fixing the PeptideHit::operator= also seems to do the trick. Memory leaks are no longer occurring according to my ASAN.

[jpfeuffer]

Yep, here safety would be more important than speed to me. I think the operations on the vectors will be more expensive than handling the pointers anyway.