Page MenuHomePhabricator

Memory leaks when calling new mitk::PointOperation
Closed, ResolvedPublic

Description

Called delete to all calls to new mitk::PointOperation.

For example:

mitk::PointOperation *doOp = new mitk::PointOperation(
  OpDESELECTPOINT, timeInMS, noPoint, position);
pointSet->ExecuteOperation( doOp );
delete doOp;

Event Timeline

xplanes added a subscriber: xplanes.

Unified patch

I discussed this already with Ingmar. In the original design, the operations mustn't get deleted since they were kept referenced somewhere else. But we already tried the patch and did not observe any strange behavior or crashes. We will check this again and then commit the fix, thanks.

Finally I could check the code. The attached patch basically adds lines with "delete doOperation;" to class mitkPointSetInteractor.cpp. Surprisingly this causes no crash after clicking Undo and Redo buttons.

I must regret the implementation of the class mitkOperation is enhanceable!
A redesign would be the best but also the most time consuming choice.

@xplanes: My guess would be, that you don't use the undo-mechanism. Right? Otherwise you should also have deleted the undoOperations.

Because the Operation objects are held in the Undo and Redo-List, deleting the objects after sending them to the DataObject is not the right way. I will modify mitkLimitedLinearUndo to free the memory once the Undo or Redo-List is cleared (Memory Leak and BUG). Then the UndoMechanism takes care of freeing the memory.

The next redesign could change mitkOperations to a general mitkOperation derived from itkLightObject and containing a PropertyList with all necessary parameters for an operation. Thus only one operation class would be there for all different operation types. Right?

Any further suggestions?

Thanks Ingmar for your explanation. You're right, we don't use Undo/Redo. I think your proposed solution is very good.

Best,
Xavi

Added wiki specification. Removed memory leak when undo is enabled. Still need to solve the case if undo is disabled.

(In reply to comment #6)

Added wiki specification. Removed memory leak when undo is enabled. Still need
to solve the case if undo is disabled.

This looks like a good candidate for a unit test. Could you describe or implement a test case that reproduces the bug, so that the issue will not reappear again?

Didn't check in changes because of issues concerning testing. To be solved soon.

[SVN revision 21810]
FIX (#3248): fixing bug that caused a runtime error when calling destructor of OperatonEvent while m_Destination was invalid; adding documentation

[SVN revision 21811]
FIX (#3248): changing clearing lists in Undo mechanism; adding new unit test to check freeing memory in Undo mechanism

[SVN revision 21831]
FIX (#3248): deleting operation objects if undo is disabled

Xavi,
this should solve the issue, right?

Best Regards,
Ingmar

Yes, I'm sure that It solves the problem. Thank you for your effort in solving this issue Ingmar.
Best,
Xavi

[SVN revision 21867]
DOC (#3248): adding documentation for enabling and disabling the undo mechanism

[SVN revision 21895]
FIX (#3248): additional reference cleanup (StateEvents and PositionEvents)