HomePhabricator
Diffusion MITK 750daa0b6d2b

Fixed crash for single time step segmentations
Needs Verification750daa0b6d2b

Description

Fixed crash for single time step segmentations

Details

Auditors
floca
Provenance
kauschAuthored on Apr 26 2019, 11:40 AM
kauschPushed on Apr 26 2019, 11:40 AM
Parents
rMITK55cb5498ee4d: Added Try-Catch For Multiple Timepoints Segmentation
Branches
Unknown
Tags
Unknown

Event Timeline

floca raised a concern with this commit.Apr 26 2019, 1:17 PM
floca added a subscriber: floca.

The fix is not good and has several problems. See comment above. If it is not clear how to handle it properly, just drop by an I can explaine.

/Modules/ImageStatistics/mitkImageStatisticsContainer.cpp
129–130

You cannot just create a container on the fly on the heap and pass it back as a reference.
This is highly likely to cause crashes due to corrupted opject states.

Don't do that.

The correct way to handle this with the current API is to either

  • caller uses try catch arround GetStatisticsForTimeStep
  • or he checks with TimeStepExists() befor retrieving the time step.

If the time step does not exist it is job of the caller to specify the fallback strategy. It is not part of the container to specify or enforce anything in this direction.

This commit now has outstanding concerns.Apr 26 2019, 1:17 PM
/Modules/ImageStatistics/mitkImageStatisticsContainer.cpp
129–130

One correction. It will not cause crash (that would be the case with smartpointers as raw pointers. Currently it will generate memory leaks.

kalali added inline comments.
/Modules/ImageStatistics/mitkImageStatisticsContainer.cpp
129–130

I second that. I also ran into this problem when using the classes for my own plugin. For testing I only added a mask at one time step -> crash.

Checking in any way if 'GetStatisticsForTimeStep' is successfully without directly accessing 'm_Histogram' seems to be the correct way to go.
You did this before with your try-catch approach. In the corresponding task you mentioned another problem (another exception?). Did you find out more about this or why did this lead to you discarding the try-catch approach?

There seems to be another exception thrown when willing the table view. The statistics table is not filled. The idea is now not to change the GetStatisticsForTimeStep functionality but handle the exception in the level above (like we started before with the try-catch)

kausch requested verification of this commit.May 10 2019, 11:29 AM
kausch marked 3 inline comments as done.

We reverted the changes according to your concerns. Instead of creating a new ImageStatisticsObject in the GetStatisticsForTimeStep method, we now check whether the time step exists before calling the method. That fixed the problem. Statistics are now only shown for the time steps that have a segmentation associated with.

This commit now requires verification by auditors.May 10 2019, 11:29 AM