1 From 0874861bcc70313c343aba5e5566ed30b69eed1c Mon Sep 17 00:00:00 2001
2 From: Stephen Kelly <steveire@gmail.com>
3 Date: Mon, 19 Dec 2016 21:13:57 +0000
4 Subject: [PATCH] QSFPM: Remove data manipulation from move handlers
5
6 Similar to the fix in the parent commit, incorrect updating of the
7 internal data structures during layout changes can lead to dangling
8 pointers being dereferenced later. Moves are treated as layoutChanges
9 by this proxy by forwarding to the appropriate method. However, data is
10 incorrectly cleared prior to that forwarding. Remove that, and let the
11 layoutChange handling take appropriate action.
12
13 Change-Id: Iee951e37152328a4e6a5fb8e5385c32a2fe4c0bd
14 Reviewed-by: David Faure <david.faure@kdab.com>
15 ---
16 src/corelib/itemmodels/qsortfilterproxymodel.cpp | 67 ++++------------------
17 .../tst_qsortfilterproxymodel.cpp | 46 +++++++++++++++
18 2 files changed, 58 insertions(+), 55 deletions(-)
19
20 diff --git a/src/corelib/itemmodels/qsortfilterproxymodel.cpp b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
21 index 3331521..226a240 100644
22 --- a/src/corelib/itemmodels/qsortfilterproxymodel.cpp
23 +++ b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
24 @@ -1418,49 +1418,27 @@ void QSortFilterProxyModelPrivate::_q_sourceRowsRemoved(
25 void QSortFilterProxyModelPrivate::_q_sourceRowsAboutToBeMoved(
26 const QModelIndex &sourceParent, int /* sourceStart */, int /* sourceEnd */, const QModelIndex &destParent, int /* dest */)
27 {
28 - Q_Q(QSortFilterProxyModel);
29 // Because rows which are contiguous in the source model might not be contiguous
30 // in the proxy due to sorting, the best thing we can do here is be specific about what
31 // parents are having their children changed.
32 // Optimize: Emit move signals if the proxy is not sorted. Will need to account for rows
33 // being filtered out though.
34
35 - saved_persistent_indexes.clear();
36 -
37 QList<QPersistentModelIndex> parents;
38 - parents << q->mapFromSource(sourceParent);
39 + parents << sourceParent;
40 if (sourceParent != destParent)
41 - parents << q->mapFromSource(destParent);
42 - emit q->layoutAboutToBeChanged(parents);
43 - if (persistent.indexes.isEmpty())
44 - return;
45 - saved_persistent_indexes = store_persistent_indexes();
46 + parents << destParent;
47 + _q_sourceLayoutAboutToBeChanged(parents, QAbstractItemModel::NoLayoutChangeHint);
48 }
49
50 void QSortFilterProxyModelPrivate::_q_sourceRowsMoved(
51 const QModelIndex &sourceParent, int /* sourceStart */, int /* sourceEnd */, const QModelIndex &destParent, int /* dest */)
52 {
53 - Q_Q(QSortFilterProxyModel);
54 -
55 - // Optimize: We only need to clear and update the persistent indexes which are children of
56 - // sourceParent or destParent
57 - qDeleteAll(source_index_mapping);
58 - source_index_mapping.clear();
59 -
60 - update_persistent_indexes(saved_persistent_indexes);
61 - saved_persistent_indexes.clear();
62 -
63 - if (dynamic_sortfilter && update_source_sort_column()) {
64 - //update_source_sort_column might have created wrong mapping so we have to clear it again
65 - qDeleteAll(source_index_mapping);
66 - source_index_mapping.clear();
67 - }
68 -
69 QList<QPersistentModelIndex> parents;
70 - parents << q->mapFromSource(sourceParent);
71 + parents << sourceParent;
72 if (sourceParent != destParent)
73 - parents << q->mapFromSource(destParent);
74 - emit q->layoutChanged(parents);
75 + parents << destParent;
76 + _q_sourceLayoutChanged(parents, QAbstractItemModel::NoLayoutChangeHint);
77 }
78
79 void QSortFilterProxyModelPrivate::_q_sourceColumnsAboutToBeInserted(
80 @@ -1522,42 +1500,21 @@ void QSortFilterProxyModelPrivate::_q_sourceColumnsRemoved(
81 void QSortFilterProxyModelPrivate::_q_sourceColumnsAboutToBeMoved(
82 const QModelIndex &sourceParent, int /* sourceStart */, int /* sourceEnd */, const QModelIndex &destParent, int /* dest */)
83 {
84 - Q_Q(QSortFilterProxyModel);
85 -
86 - saved_persistent_indexes.clear();
87 -
88 QList<QPersistentModelIndex> parents;
89 - parents << q->mapFromSource(sourceParent);
90 + parents << sourceParent;
91 if (sourceParent != destParent)
92 - parents << q->mapFromSource(destParent);
93 - emit q->layoutAboutToBeChanged(parents);
94 -
95 - if (persistent.indexes.isEmpty())
96 - return;
97 - saved_persistent_indexes = store_persistent_indexes();
98 + parents << destParent;
99 + _q_sourceLayoutAboutToBeChanged(parents, QAbstractItemModel::NoLayoutChangeHint);
100 }
101
102 void QSortFilterProxyModelPrivate::_q_sourceColumnsMoved(
103 const QModelIndex &sourceParent, int /* sourceStart */, int /* sourceEnd */, const QModelIndex &destParent, int /* dest */)
104 {
105 - Q_Q(QSortFilterProxyModel);
106 -
107 - qDeleteAll(source_index_mapping);
108 - source_index_mapping.clear();
109 -
110 - update_persistent_indexes(saved_persistent_indexes);
111 - saved_persistent_indexes.clear();
112 -
113 - if (dynamic_sortfilter && update_source_sort_column()) {
114 - qDeleteAll(source_index_mapping);
115 - source_index_mapping.clear();
116 - }
117 -
118 QList<QPersistentModelIndex> parents;
119 - parents << q->mapFromSource(sourceParent);
120 + parents << sourceParent;
121 if (sourceParent != destParent)
122 - parents << q->mapFromSource(destParent);
123 - emit q->layoutChanged(parents);
124 + parents << destParent;
125 + _q_sourceLayoutChanged(parents, QAbstractItemModel::NoLayoutChangeHint);
126 }
127
128 /*!
129 diff --git a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
130 index 6b98d9f..7b6c470 100644
131 --- a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
132 +++ b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
133 @@ -146,6 +146,7 @@ private slots:
134 void filterHint();
135
136 void sourceLayoutChangeLeavesValidPersistentIndexes();
137 + void rowMoveLeavesValidPersistentIndexes();
138
139 protected:
140 void buildHierarchy(const QStringList &data, QAbstractItemModel *model);
141 @@ -4307,5 +4308,50 @@ void tst_QSortFilterProxyModel::sourceLayoutChangeLeavesValidPersistentIndexes()
142 QVERIFY(persistentIndex.parent().isValid());
143 }
144
145 +void tst_QSortFilterProxyModel::rowMoveLeavesValidPersistentIndexes()
146 +{
147 + DynamicTreeModel model;
148 + Q_SET_OBJECT_NAME(model);
149 +
150 + QList<int> ancestors;
151 + for (auto i = 0; i < 5; ++i)
152 + {
153 + Q_UNUSED(i);
154 + ModelInsertCommand insertCommand(&model);
155 + insertCommand.setAncestorRowNumbers(ancestors);
156 + insertCommand.setStartRow(0);
157 + insertCommand.setEndRow(0);
158 + insertCommand.doCommand();
159 + ancestors.push_back(0);
160 + }
161 +
162 + QSortFilterProxyModel proxy1;
163 + proxy1.setSourceModel(&model);
164 + Q_SET_OBJECT_NAME(proxy1);
165 +
166 + proxy1.setFilterRegExp("1|2");
167 +
168 + auto item5 = model.match(model.index(0, 0), Qt::DisplayRole, "5", 1, Qt::MatchRecursive).first();
169 + auto item3 = model.match(model.index(0, 0), Qt::DisplayRole, "3", 1, Qt::MatchRecursive).first();
170 +
171 + Q_ASSERT(item5.isValid());
172 + Q_ASSERT(item3.isValid());
173 +
174 + QPersistentModelIndex persistentIndex = proxy1.match(proxy1.index(0, 0), Qt::DisplayRole, "2", 1, Qt::MatchRecursive).first();
175 +
176 + ModelMoveCommand moveCommand(&model, 0);
177 + moveCommand.setAncestorRowNumbers(QList<int>{0, 0, 0, 0});
178 + moveCommand.setStartRow(0);
179 + moveCommand.setEndRow(0);
180 + moveCommand.setDestRow(0);
181 + moveCommand.setDestAncestors(QList<int>{0, 0, 0});
182 + moveCommand.doCommand();
183 +
184 + // Calling parent() causes the internalPointer to be used.
185 + // Before fixing QTBUG-47711 (moveRows case), that could be
186 + // a dangling pointer.
187 + QVERIFY(persistentIndex.parent().isValid());
188 +}
189 +
190 QTEST_MAIN(tst_QSortFilterProxyModel)
191 #include "tst_qsortfilterproxymodel.moc"
|
