Fixes crash when hiding devices. The crash is caused by
KStandardItem::setDataValue which calls the
KStandardItemModel::onItemChanged function, and that function will
delete the KStandardItem if the data value being set is the hidden
attribute being set to true. So therefore this function call has to be
last.
Details
- Reviewers
elvisangelaccio - Group Reviewers
Dolphin
Right click a device in the places panel and select hide
Right click the places panel and select show hidden
Right click the hidden device and select show
Right click the same device and select hide
BUG: 403064
Diff Detail
- Repository
- R318 Dolphin
- Branch
- fix_crash_when_hiding_devices (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 11115 Build 11133: arc lint + arc unit
Hmm, I can't reproduce the crash following the test plan.
Right click the hidden device and select show
Did you mean "uncheck Hide" ?
That said, the fix looks correct. An unit test for this crash would be awesome, if you have spare time.
Hi @elvisangelaccio , I have been looking into writing a unit test for the crash, but it turns out we already have one: PlacesModelItemTest::testHideItem. But, the test only triggers the crash if it is run before the testPlaceItem test case. I have been digging in the code to figure out why the test cases are coupled in this way, but I haven't figured it out, so could you take a look? Here is a diff that triggers the crash:
diff --git a/src/tests/placesitemmodeltest.cpp b/src/tests/placesitemmodeltest.cpp index fac0931a6..05aa02064 100644 --- a/src/tests/placesitemmodeltest.cpp +++ b/src/tests/placesitemmodeltest.cpp @@ -63,13 +63,13 @@ private slots: void testModelSort(); void testGroups(); void testDeletePlace(); + void testHideItem(); void testPlaceItem_data(); void testPlaceItem(); void testTearDownDevice(); void testDefaultViewProperties_data(); void testDefaultViewProperties(); void testClear(); - void testHideItem(); void testSystemItems(); void testEditBookmark(); void testEditAfterCreation();
So if I revert the crash fix and run the test like this valgrind bin/placesitemmodeltest I see the crash:
==6910== Invalid read of size 8 ==6910== at 0x5166A43: KStandardItem::setDataValue(QByteArray const&, QVariant const&) (kstandarditem.cpp:118) ==6910== by 0x178D2B: PlacesItem::setHidden(bool) (placesitem.cpp:96) ==6910== by 0x1575C0: PlacesItemModelTest::testHideItem() (placesitemmodeltest.cpp:520) ==6910== by 0x16AFD3: PlacesItemModelTest::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (placesitemmodeltest.moc:159) ==6910== by 0xC64C900: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (in /usr/lib64/libQt5Core.so.5.11.3) ==6910== by 0x4E5236B: ??? (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x4E530DC: ??? (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x4E53650: ??? (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x4E53B0A: QTest::qRun() (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x4E53E2A: QTest::qExec(QObject*, int, char**) (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x16AEC3: main (placesitemmodeltest.cpp:947) ==6910== Address 0x2319abd8 is 24 bytes inside a block of size 136 free'd ==6910== at 0x4C3083B: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==6910== by 0x1787ED: PlacesItem::~PlacesItem() (placesitem.cpp:51) ==6910== by 0x5173C74: KStandardItemModel::removeItem(int) (kstandarditemmodel.cpp:115) ==6910== by 0x17FF7C: PlacesItemModel::onSourceModelDataChanged(QModelIndex const&, QModelIndex const&, QVector<int> const&) (placesitemmodel.cpp:569) ==6910== by 0x1862A5: QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1, 2>, QtPrivate::List<QModelIndex const&, QModelIndex const&, QVector<int> const&>, void, void (PlacesItemModel::*)(QModelIndex const&, QModelIndex const&, QVector<int> const&)>::call(void (PlacesItemModel::*)(QModelIndex const&, QModelIndex const&, QVector<int> const&), PlacesItemModel*, void**) (qobjectdefs_impl.h:134) ==6910== by 0x185A61: void QtPrivate::FunctionPointer<void (PlacesItemModel::*)(QModelIndex const&, QModelIndex const&, QVector<int> const&)>::call<QtPrivate::List<QModelIndex const&, QModelIndex const&, QVector<int> const&>, void>(void (PlacesItemModel::*)(QModelIndex const&, QModelIndex const&, QVector<int> const&), PlacesItemModel*, void**) (qobjectdefs_impl.h:167) ==6910== by 0x1843C0: QtPrivate::QSlotObject<void (PlacesItemModel::*)(QModelIndex const&, QModelIndex const&, QVector<int> const&), QtPrivate::List<QModelIndex const&, QModelIndex const&, QVector<int> const&>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:396) ==6910== by 0xC66596E: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib64/libQt5Core.so.5.11.3) ==6910== by 0xC5FB45B: QAbstractItemModel::dataChanged(QModelIndex const&, QModelIndex const&, QVector<int> const&) (in /usr/lib64/libQt5Core.so.5.11.3) ==6910== by 0x56BBC63: KFilePlacesModel::setPlaceHidden(QModelIndex const&, bool) (in /usr/lib64/libKF5KIOFileWidgets.so.5.56.0) ==6910== by 0x17D77B: PlacesItemModel::onItemChanged(int, QSet<QByteArray> const&) (placesitemmodel.cpp:189) ==6910== by 0x5166A3E: KStandardItem::setDataValue(QByteArray const&, QVariant const&) (kstandarditem.cpp:117) ==6910== Block was alloc'd at ==6910== at 0x4C2F77F: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==6910== by 0x17EB09: PlacesItemModel::addItemFromSourceModel(QModelIndex const&) (placesitemmodel.cpp:392) ==6910== by 0x18088F: PlacesItemModel::loadBookmarks() (placesitemmodel.cpp:628) ==6910== by 0x17CC34: PlacesItemModel::PlacesItemModel(QObject*) (placesitemmodel.cpp:66) ==6910== by 0x151779: PlacesItemModelTest::init() (placesitemmodeltest.cpp:238) ==6910== by 0x16AF5C: PlacesItemModelTest::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (placesitemmodeltest.moc:152) ==6910== by 0xC64C900: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (in /usr/lib64/libQt5Core.so.5.11.3) ==6910== by 0x4E526FB: ??? (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x4E530DC: ??? (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x4E53650: ??? (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x4E53B0A: QTest::qRun() (in /usr/lib64/libQt5Test.so.5.11.3) ==6910== by 0x4E53E2A: QTest::qExec(QObject*, int, char**) (in /usr/lib64/libQt5Test.so.5.11.3) ==6910==
My hunch is that PlacesItemModel uses the DolphinPlacesModelSingleton::placesModel as it's source model, and since that is a static variable it's state is carried across between tests. But I haven't figured out if this is indeed the problem, or if it is something else.
I really hope you can help clarify this mystery :)
Still not crashing for me, even if I apply this diff...
Can anyone else try to reproduce the crash following the Test Plan? @ngraham maybe?
Could you try and move the testHideItem to be the first test? Also make sure to run placesitemmodeltest using valgrind, i.e. valgrind placesitemmodeltest
Right, I was able to reproduce the stacktrace using valgrind.
src/kitemviews/kstandarditem.cpp | ||
---|---|---|
114–115 | While this change fixes the crash, I consider it a workaround as it's relying on an implementation detail of the model. It doesn't fix the actual problem, which I think is in PlacesItemModel::onSourceModelDataChanged(). Can you try to delay the removeItem() call (at line 569) using a QTimer::singleShot() ? Basically, we should do what dolphin used to do with the old PlacesItemModel implementation: void PlacesItemModel::onItemChanged(int index, const QSet<QByteArray>& changedRoles) { ... if (changedRoles.contains("isHidden")) { if (!m_hiddenItemsShown && changedItem->isHidden()) { m_hiddenItemToRemove = index; QTimer::singleShot(0, this, static_cast<void (PlacesItemModel::*)()>(&PlacesItemModel::hideItem)); } } } (commit da6f8fe0862585287 dropped the QTimer and probably introduced this crash). |
src/kitemviews/kstandarditem.cpp | ||
---|---|---|
114–115 | Yeah, I tend to agree :D But I was actually thinking if it would be nicer if the items were QObjects, and then we could use deleteLater? Using a timer still kind of relies on implementation detail knowledge... |
src/kitemviews/kstandarditem.cpp | ||
---|---|---|
114–115 | Feel free to try :) |
Change KStandardItem to derive from QObject, and use deleteLater when removing an item.
src/kitemviews/kstandarditem.cpp | ||
---|---|---|
114–115 | Ok, I have now changed KStandardItem to derive from QObject instead, and then call deleteLater when removing an item. This also fixes the crash and I think it is a _much_ nicer fix. Also this removes some of the custom parent/children code from KStandardItem |
Please also update the commit message (the fix is now different).
src/kitemviews/kstandarditem.cpp | ||
---|---|---|
48 | Shouldn't this be QObject(item->parent()) ? |