Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-47711

QSortFilterProxyModel policy on invalidating QModelndexes isn't consistent, can lead to crashes

    XMLWordPrintable

Details

    • Bug
    • Resolution: Unresolved
    • P2: Important
    • None
    • 5.2.0
    • Core: Item Models
    • None
    • Windows 7, Visual Studio 2013 x64

    Description

      QSortFilterProxyModel does two conflicting things in response to a 'layoutChanged' signal from its source model (the logic for this response is in QSortFilterProxyModelPrivate::_q_sourceLayoutChanged).

      1. Clears QSortFilterProxyModelPrivate::source_index_mapping, freeing all allocated QSortFilterProxyModelPrivate::Mapping objects, therefore invalidating all QModelIndexes for items in this QSortFilterProxyModel (because these QModelIndexes reference QSortFilterProxyModelPrivate::Mapping objects through their internalPointer()).
      2. Only fires 'layoutChanged' when the change in layout in the source model affects the proxy model exposed by the QSortFilterProxyModel.

      (2.) means that QSortFilterProxyModel is working under the assumption that users of a QAbstractItemModel can assume that a 'layoutChanged' signal only invalidates the QModelIndexes affected by the layout change. But then the behavior in (1.) violates this assumption by invalidating all the QModelIndexes for this QSortFilterProxyModel.

      In general this means that the assumptions that QSortFilterProxyModel makes about its source model are not satisfied by QSortFilterProxyModel itself. This means that when nesting a QSortFilterProxyModel inside another QSortFilterProxyModel there's a risk of crashing due to the use of invalid QModelIndexes in the outer QSortFilterProxyModel (this setup is just how I came across this issue, and how the issue is reproduced in the code below).

      Qt documentation on when QModelIndex values obtained from a QAbstractItemModel need to be discarded isn't very clear I feel, so to me it's not obvious if the behavior in (1.) or (2.) needs to change.

      To reproduce issue, run this app and click the single button in the app:

      #include <QApplication>
      #include <QMainWindow>
      #include <QAbstractItemModel>
      #include <QTreeView>
      #include <QHBoxLayout>
      #include <QPushButton>
      #include <QSortFilterProxyModel>
      
      // A tree item model that has only one branch, and that branch is exactly 'depth' items deep
      // (this tree is needed to reproduce the issue because we need a tree which fires 'layoutAboutToBeChanged'/'layoutChanged' when it's being edited,
      // and this seemed like the easiest way to get that)
      class SingleBranchTreeModel : public QAbstractItemModel
      {
      	public:
      		SingleBranchTreeModel(quint32 depth, QObject * parent = 0) : QAbstractItemModel(parent), m_depth(depth) {}
      
      		virtual int columnCount(const QModelIndex & parent = QModelIndex()) const { return 1; }
      
      		virtual int rowCount(const QModelIndex & parent = QModelIndex()) const
      		{
      			// all indices have one child row, expect those that are deeper than 'depth'
      			quintptr parentId = (parent.isValid()) ? parent.internalId() : 0;
      			return (parentId < m_depth) ? 1 : 0;
      		}
      
      		virtual QVariant data(const QModelIndex & index, int role = Qt::DisplayRole) const
      		{
      			if (role == Qt::DisplayRole)
      			{
      				// return the item's id as its display role
      				return QString::number(index.internalId());
      			}
      			else return QVariant();
      		}
      
      		virtual QModelIndex index(int row, int column, const QModelIndex & parent = QModelIndex()) const
      		{
      			quintptr parentId = (parent.isValid()) ? parent.internalId() : 0;
      			if (parentId < m_depth)
      			{
      				return createIndex(0, 0, parentId + 1);
      			}
      			else return QModelIndex();
      		}
      
      		virtual QModelIndex parent(const QModelIndex & index) const
      		{
      			quintptr parentId = index.internalId() - 1;
      			if (parentId == 0)
      			{
      				return QModelIndex();
      			}
      			else return createIndex(0, 0, index.internalId() - 1);
      		}
      
      		quint32 getDepth() { return m_depth; }
      
      		void setDepth(quint32 depth)
      		{
      			quint32 parentIdWithLayoutChange = (m_depth < depth) ? m_depth : depth;
      
      			QList<QPersistentModelIndex> parentsOfLayoutChange;
      			if (parentIdWithLayoutChange > 0)
      			{
      				parentsOfLayoutChange.push_back(createIndex(0, 0, parentIdWithLayoutChange));
      			}
      
      			layoutAboutToBeChanged(parentsOfLayoutChange);
      
      			m_depth = depth;
      
      			layoutChanged(parentsOfLayoutChange);
      		}
      
      	private:
      
      		quint32 m_depth;
      };
      
      // A proxy model that filters out the entire source model except items that are at a certain depth or less
      class OnlyKeepIndicesThisDeep : public QSortFilterProxyModel
      {
      	public:
      
      		OnlyKeepIndicesThisDeep(quint32 maxDepth, QObject* parent = 0) : QSortFilterProxyModel(parent), m_maxDepth(maxDepth) {}
      
      	protected:
      
      		virtual bool filterAcceptsRow(int sourceRow, const QModelIndex& sourceParent) const
      		{
      			return indexDepth(sourceParent) < m_maxDepth;
      		}
      
      		static quint32 indexDepth(QModelIndex index)
      		{
      			return (index.isValid()) ? 1 + indexDepth(index.parent()) : 0;
      		}
      
      		quint32 m_maxDepth;
      };
      
      int main( int argc, char *argv[] )
      {
      	QApplication app(argc, argv);
      
      	QMainWindow mainWindow;
      
      	QWidget* mainWidget = new QWidget;
      	{
      		// create a tree that only has one branch, which is 4 levels deep
      		SingleBranchTreeModel* singleBranchTreeModel = new SingleBranchTreeModel(4);
      		singleBranchTreeModel->setParent(mainWidget);
      
      		// create a proxy that only shows the first 2 levels of this tree
      		OnlyKeepIndicesThisDeep* proxyWithMaxDepthOf2 = new OnlyKeepIndicesThisDeep(2);
      		proxyWithMaxDepthOf2->setSourceModel(singleBranchTreeModel);
      		proxyWithMaxDepthOf2->setParent(singleBranchTreeModel);
      
      		// create another proxy that only shows the first level of the previous proxy
      		OnlyKeepIndicesThisDeep* proxyWithMaxDepthOf1 = new OnlyKeepIndicesThisDeep(1);
      		proxyWithMaxDepthOf1->setSourceModel(proxyWithMaxDepthOf2);
      		proxyWithMaxDepthOf1->setParent(proxyWithMaxDepthOf2);
      
      
      		//create views that display all three of these models
      		QTreeView* treeViewOfSource = new QTreeView;
      		treeViewOfSource->setHeaderHidden(true);
      		treeViewOfSource->setModel(singleBranchTreeModel);
      
      		QTreeView* treeViewOfProxyWithMaxDepthOf2 = new QTreeView;
      		treeViewOfProxyWithMaxDepthOf2->setHeaderHidden(true);
      		treeViewOfProxyWithMaxDepthOf2->setModel(proxyWithMaxDepthOf2);
      
      		QTreeView* treeViewOfProxyWithMaxDepthOf1 = new QTreeView;
      		treeViewOfProxyWithMaxDepthOf1->setHeaderHidden(true);
      		treeViewOfProxyWithMaxDepthOf1->setModel(proxyWithMaxDepthOf1);
      
      
      		// create a button which prunes the tree, which is what's needed to happen to reproduce the issue
      		QPushButton* pruneButton = new QPushButton("Prune Source Tree (Push to crash!)");
      		{
      			QObject::connect(pruneButton, &QAbstractButton::clicked,
      				[singleBranchTreeModel]()
      				{
      					singleBranchTreeModel->setDepth(singleBranchTreeModel->getDepth() - 1);
      					// in response to this change, all QModelIndexes from proxyWithMaxDepthOf2 should be discarded,
      					// but proxyWithMaxDepthOf1 doesn't do this, and a crash should result when a free'd QSortFilterProxyModelPrivate::Mapping is next accessed
      				});
      		}
      
      		// put the 3 views, and the button in a Horizontal layout
      		QHBoxLayout* mainLayout = new QHBoxLayout;
      		mainLayout->addWidget(treeViewOfSource);
      		mainLayout->addWidget(treeViewOfProxyWithMaxDepthOf2);
      		mainLayout->addWidget(treeViewOfProxyWithMaxDepthOf1);
      		mainLayout->addWidget(pruneButton);
      
      		mainWidget->setLayout(mainLayout);
      	}
      	mainWindow.setCentralWidget(mainWidget);
      
      	mainWindow.show();
      
      	return app.exec();
      }
      

      Attachments

        Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            People

              peppe Giuseppe D'Angelo
              jacob.enget Jacob Enget
              Votes:
              2 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

                Created:
                Updated:

                Gerrit Reviews

                  There are no open Gerrit changes