Fix QSortFilterProxyModelPrivate::updateChildrenMapping crash in libtaskmanager
ClosedPublic

Authored by hein on Aug 4 2017, 7:11 PM.

Details

Summary

TaskGroupingProxyModel uses a simple QVector<QVector<int>> populated
with source model row indices to represent the task group tree. To
implement QAbstractItemModel::parent(), its implementation of index()
encodes row indices of the top-level vector into the internal ids of
child item model indices. This allows parent() to produce the parent
model index by simply decoding the parent row from the passed-in child
index and call index() with that row.

Top-level row indices shift up and down as the list of top-level items
changes, invalidating those internal ids. QModelIndex is not meant to
be stored, and the proxy model does take care of updating any persis-
tent model indexes with new ids, so this should be fine.

However, where it falls apart is that as internal ids are invalidated,
a QSortFilterProxyModel on top of this proxy (i.e. TasksModel) may end
up with multiple indexes with identical internal ids in its mappings,
causing it to mess up its mappings as it uses them (e.g. taking things
from them). This causes the often-reported crash/assert there.

The fix is to refactor index()/parent() not to rely on row indices as
internal ids, but instead use pointers to internal data structures
instead.

This patch achieves this by changing the map to QVector<QVector<int> *>.
This screams fugly, but the alternative would basically just be to
create some wrapper struct to hide the fugly appeareance a little,
which I don't think is worth it.

On the flip side, it saves a QVector::replace() call as a multable
vector iterator can work directly on a vector without making a copy,
and it's now no longer necessary to manually update the persistent
model indices beyond what endRemoveRows() does implicitly.

BUG:381006

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
hein created this revision.Aug 4 2017, 7:11 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 4 2017, 7:11 PM
hein updated this revision to Diff 17732.Aug 4 2017, 7:19 PM

Forgot to actually drop the blocks updating QPMI's

hein updated this revision to Diff 17733.Aug 4 2017, 7:22 PM

Slightly nicer commit message.

hein edited the summary of this revision. (Show Details)Aug 4 2017, 7:23 PM

Updated description to match commit message because arc is stupid.

davidedmundson edited edge metadata.Aug 4 2017, 9:18 PM

Looks good. 1 tiny change needed I think.

Also some code comments on the QVector would be useful for future people.

libtaskmanager/taskgroupingproxymodel.cpp
41

Cleanup on destruction.

(which sounds like the name of a new Megadeth single)

465

(I know this existing code)

why 1?
shouldn't this should be currentSize + extraChildren.count()

521

I think we should assert after this if.

hein marked 3 inline comments as done.Aug 4 2017, 10:14 PM
hein added inline comments.
libtaskmanager/taskgroupingproxymodel.cpp
41

Will do.

465

This code is breaking a group apart, and this transaction specifically removes all the children from a group parent, leaving only the top-level item. In the row map this means an element QVector with only one element, hence it's resized to size 1.

521

Will do.

hein updated this revision to Diff 17737.Aug 4 2017, 10:19 PM
hein marked 3 inline comments as done.
  • qDeleteAll the new pointer vector on destruction.
  • Add a Q_ASSERT to catch data corruption.
davidedmundson accepted this revision.Aug 5 2017, 8:53 AM
This revision is now accepted and ready to land.Aug 5 2017, 8:53 AM
This revision was automatically updated to reflect the committed changes.