Page MenuHomePhabricator

Crash when changing parent data nodes in Data Manager
Closed, ResolvedPublic

Description

Branch develop 9a29ffc60b2e5273957b65225184c45fef52ed95
Allow changing of parent node (Preferences of Data Manager).

  1. Open Pic3D
  2. Creat segmentation
  3. Save segmentation as nrrd
  4. Remove files from Data Manager
  5. Open Pic3D
  6. Open segmenation
  7. In Data Manager add segmentation as a child of Pic3D.
  8. Results in a crash itkSmartPointer::Register() (read access violation)

Revisions and Commits

rMITK MITK
Restricted Differential Revision
Restricted Differential Revision

Event Timeline

gaoh triaged this task as Normal priority.Jul 14 2020, 11:33 AM
gaoh created this task.

I found out that this is a basic problem of changing of parent nodes. It can also be reproduced in a much more simpler way:

  1. Open Pic3D
  2. Open US4DCyl
  3. Make one image the child node of the other image
  4. Crash

This did not happen in MITK v2018.04.2

floca raised the priority of this task from Normal to High.Jul 17 2020, 12:49 PM
kislinsk renamed this task from Adding Segmentation to Parent Crashes to Crash when changing parent data nodes in Data Manager.Jul 20 2020, 10:57 AM

Sorry for blaming here but I had some suspicions and indeed, the bug was introduced here: 8dea40a4a757

Currently I can't see if this really introduced the bug or made an already existing bug visible.

My first guess is:
in line 305 of QmitkDataStorageTreeModel::dropMimeData
mitk::DataNode *droppedNode = (*diIter)->GetDataNode();
the dropped node is returned, which is invalidated after line 307:
dataStorage->Remove(droppedNode);
which leads to an error in line 308:
dataStorage->Add(droppedNode, dropOntoNode);
because droppedNode does not exist anymore.

This was not a problem before, because the raw pointer still existed but now the weak pointer frees the memory? Something like that.
One solution for this could be to replace the raw pointer in line 305 with a smart pointer
mitk::DataNode::Pointer droppedNode = (*diIter)->GetDataNode();

But maybe I'm wrong and something is not fine with QmitkDataStorageTreeModelInternalItem::GetDataNode().

Btw.: QmitkDataStorageTreeModel::dropMimeData is waaay to long and complicated - no wonder there might be some bugs hidden.

I agree that the correct solution is to use a smart pointer in line 305 instead of a raw pointer. Ralf did not introduce a bug but made another one visible. :-)

I looked into the code and aggree on both:

  1. Using a smartpointer at L305 makes sense, not using it is hazardous.
  2. Ralf does not introduce errors ๐Ÿ˜„

Ralf does not introduce errors ๐Ÿ˜„

This reads like a general statement ๐Ÿ˜›

kalali added a revision: Restricted Differential Revision.Jul 22 2020, 2:49 PM

Ralf does not introduce errors ๐Ÿ˜„

This reads like a general statement ๐Ÿ˜›

Start at 16 seconds, end at 30 seconds (sorry @floca):

Start at 16 seconds, end at 30 seconds (sorry @floca):

ROFL (ralf on the floor laughing)
๐Ÿ˜†