Page MenuHomePhabricator

Workbench crash on simple tree model update
Open, NormalPublic

Description

The new selection concept (see T23751) provides basic classes to introduce the new mechanism. One of the classes is the QmitkDataStorageSimpleTreeModel used by QmitkDataStorageTreeInspector.
This class is an implementation of the QmitkAbstractDataStorageModel and provides an implementation of the UpdateModelData-function and the DataStorageChanged-function.
However, if DataStorageChanged is invoked, which in turn invokes UpdateModelData, the workbench crashes in int QmitkDataStorageTreeModelInternalItem::GetIndex() const.

UpdateModelData is also invoked on initialization when e.g. the data storage is set for the first time. Here no crash happens.

Edit:
I couldn't reproduce the problem, because I was not able to reproduce the workflow I used back in 2019. However, I found a way to reproduce the basic problem:

  1. enable the data storage viewer test plugin
  2. open MITK and open the data storage viewer test plugin
  3. load a data node and create a child segmentation node
  4. show / select the data storage viewer test plugin
  5. remove the top-level image node inside the datamanager

-> Crash inside QmitkDataStorageTreeModelInternalItem::IndexOfChild, called from QmitkDataStorageTreeModelInternalItem::GetIndex()

The data storage viewer test plugin is needed to have an additional QmitkDataStorageSimpleTreeModel that reacts to the node removed event.

However, my proposed fix does not work here. So maybe this is a completely different bug.
The QmitkDataStorageSimpleTreeModel uses the UpdateModelData-function to reset its data nodes. This is done by retrieving all nodes from the data storage. Here it seems as if the removed node still exists.

Main message to take away: We should define clear tests for the data storage models and views and test all functionality.

Event Timeline

kalali triaged this task as Normal priority.Jun 12 2019, 4:06 PM
kalali created this task.

Possible solutions:

  1. Add
if (!parentItem)
    return QModelIndex();

to QModelIndex QmitkDataStorageSimpleTreeModel::parent(const QModelIndex &child) const

  1. Remove
if (m_Root)
{
  m_Root->Delete();
}

from void QmitkDataStorageSimpleTreeModel::DataStorageChanged(). This will lead to unreferenced pointers so I suggest to work with smart pointers (for this QmitkDataStorageTreeModelInternalItem needs to be modified).

One reason could be that a child item that is removed by calling Delete on the root item is not removed from m_TreeItems and thus will still be returned when using TreeItemFromIndex inside QmitkDataStorageSimpleTreeModel::parent. Just a quick guess.

Isn't (1) a requirement anyways? Let me just look it up in the Qt documentation...

Note that, since the root item in the model will not have a parent, this function will return zero in that case. We need to ensure that the model handles this case correctly when we implement the TreeModel::parent() function.

@floca : Ilooked into this again and found the following:
After a node has been removed, inside QmitkDataStorageSimpleTreeModel::NodeRemoved the function QmitkDataStorageSimpleTreeModel::UpdateModelData is called. Here the data storage pointer is used to retrieve the data node subset of a given node predicate. The returned nodeset still returns 6 nodes in my case, although only 5 should exists, since one node was removed.
The node still exists in the data storage, because the signal for a node being removed (RemoveNodeEvent) is sent before the actual removal of the node from the list of nodes inside StandaloneDataStorage (data member m_SourceNodes)).

This is a problem because inside the AddNodeInternal-function the tree is rebuilt by checking the parent-child-node hierarchy and adding tree-items accordingly. However, the hierarchy at that time is incorrect and already outdated.

Also: Is calling UpdateModelData the right thing to do? It will add the nodes to the member variable m_TreeItems, but this is a list and already contains the previously existing (not removed) node items, so they will be contained multiple times?
What exactly is m_TreeItems used for? Something like this is not used inside the comparable QmitkDataStorageTreeModel.

My suggestion:

I quickly tested the second suggestion and I was not able to reproduce the bug anymore.

I quickly tested the second suggestion and I was not able to reproduce the bug anymore.

In the class description it is actually explicitly stated:

If a tree item A is removed this class does not attach children of A to the parent of A.
Instead the complete tree representation is updated. This was changed on purpose because otherwise the internal
representation of the model would not reflect the data storage graph anymore.

@floca What was the reason for this: What does "would not reflect the data storage graph anymore" mean? Does the classic / original QmitkDataStorgageTreeModel have any drawbacks because it changes the "internal representation" by changing the parent?

I'm not sure if this is the reason (T24348: Invalid pointer exception due to invalid QModelIndex in QmitkDataStorageTreeModel) for the decision. I would go the route to deprecate the QmitkDataStorageModel and replace it with the QmitkDataStorageSimpleTreeModel. However, the QmitkDataStorageSimpleTreeModel probably needs to be extended (e.g. dropping, changing hierarchy). Do we already have a task for this / should we tackle this in this release-phase?
I stumbled upon these different tasks because of T26394: [Render window manager] Provide tree view / model, which is related to the mxnMultiWidget.

@floca : Ilooked into this again and found the following:
This is a problem because inside the AddNodeInternal-function the tree is rebuilt by checking the parent-child-node hierarchy and adding tree-items accordingly. However, the hierarchy at that time is incorrect and already outdated.

Good that you identified the problem.

Also: Is calling UpdateModelData the right thing to do?

Yes.

but this is a list and already contains the previously existing (not removed) node items, so they will be contained multiple times?

No it won't. AddNodeInternal checks that and does an early-out if the node is already there.

What exactly is m_TreeItems used for?

Please check the documentation of the variable.

/**helper structure to check, if a tree item is really part of the model.
Prefered over iterating over the tree by hand because we can use std::find.*/

My suggestion:

I quickly tested the second suggestion and I was not able to reproduce the bug anymore.

I strongly dislike the attempt to reconstruct the storage tree structure in the model. It is not the business of the model to reimplement logic of the datastorage. This attampt lead to several problems in the old model and is illposed. Further the data storage itself is theoretically an simple tree. It is a graph. So its representation could be completly different after delete.
Given your observation with the event. The only proper way I see is to add an additional event that is triggered post removal from the data storage (but when the node still exists). So you can choose if you want to be informed before or after the removal.

@floca What was the reason for this: What does "would not reflect the data storage graph anymore" mean? Does the classic / original QmitkDataStorgageTreeModel have any drawbacks because it changes the "internal representation" by changing the parent?

See my other comment. The datastorage is a graph not a tree. So it could happen that a tree construction look completly different after a node is removed (e.g. the children could now be top level or by children of another node; because the tree only shows one parent-child relation).

I'm not sure if this is the reason (T24348: Invalid pointer exception due to invalid QModelIndex in QmitkDataStorageTreeModel) for the decision. I would go the route to deprecate the QmitkDataStorageModel and replace it with the QmitkDataStorageSimpleTreeModel.
However, the QmitkDataStorageSimpleTreeModel probably needs to be extended (e.g. dropping, changing hierarchy). Do we already have a task for this / should we tackle this in this release-phase?

This should be carefully discussed. I think some of the features QmitkDataStorageModel does not belong in a model.

I stumbled upon these different tasks because of T26394: [Render window manager] Provide tree view / model, which is related to the mxnMultiWidget.

Do you realy need the QmitkDataStorageModel feature set for your purposes? If not, I would not open the can of worms and work on the deprecation of QmitkDataStorageModel. I think it is currently not worth it just for the sake of having it out.
What would you need on top of the QmitkDataStorageSimpleTreeModel (when the remove node bug is fixed) as feature?

I'm not sure if this is the reason (T24348: Invalid pointer exception due to invalid QModelIndex in QmitkDataStorageTreeModel) for the decision. I would go the route to deprecate the QmitkDataStorageModel and replace it with the QmitkDataStorageSimpleTreeModel.
However, the QmitkDataStorageSimpleTreeModel probably needs to be extended (e.g. dropping, changing hierarchy). Do we already have a task for this / should we tackle this in this release-phase?

This should be carefully discussed. I think some of the features QmitkDataStorageModel does not belong in a model.

The idea is not to add code to the QmitkDataStorageSimpleTreeModel but to provide an additional subclass, e.g. QmitkDataStorageDnDTreeModel that provides the drag and drop functionality.
Didn't we once talk about using the new modesl / inspectors for the datamanager view? What is your recommendation for the data manager view?

I stumbled upon these different tasks because of T26394: [Render window manager] Provide tree view / model, which is related to the mxnMultiWidget.

Do you realy need the QmitkDataStorageModel feature set for your purposes? If not, I would not open the can of worms and work on the deprecation of QmitkDataStorageModel. I think it is currently not worth it just for the sake of having it out.
What would you need on top of the QmitkDataStorageSimpleTreeModel (when the remove node bug is fixed) as feature?

See T26394#225555: I need to check again what the problem was but I do not need it right now - mainly because one part of working on the mxn-multiwidget is also to find a different mechanism to select data nodes for the render windows - not using a "render-window specific data manager" or a "render window graph", but something else.