Fixes crash when hiding devices
AbandonedPublic

Authored by hallas on Mar 24 2019, 7:00 PM.

Details

Reviewers
elvisangelaccio
Group Reviewers
Dolphin
Summary

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.

Test Plan

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
hallas created this revision.Mar 24 2019, 7:00 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMar 24 2019, 7:00 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
hallas requested review of this revision.Mar 24 2019, 7:00 PM

Hmm, I can't reproduce the crash following the test plan.

Right click the hidden device and select show

Did you mean "uncheck Hide" ?

elvisangelaccio added a comment.EditedMar 24 2019, 8:39 PM

That said, the fix looks correct. An unit test for this crash would be awesome, if you have spare time.

That said, the fix looks correct. An unit test for this crash would be awesome, if you have spare time.

Agree :D I will look into it and update the commit

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 :)

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();

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?

I'm afraid I cannot reproduce the crash either.

Could you try and move the testHideItem to be the first test? Also make sure to run placesitemmodeltest using valgrind, i.e. valgrind placesitemmodeltest

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).

hallas added inline comments.Apr 1 2019, 1:27 PM
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...

elvisangelaccio added inline comments.Apr 6 2019, 4:29 PM
src/kitemviews/kstandarditem.cpp
114–115

Feel free to try :)

hallas updated this revision to Diff 56759.Apr 22 2019, 5:45 PM

Change KStandardItem to derive from QObject, and use deleteLater when removing an item.

hallas marked an inline comment as done.Apr 22 2019, 5:47 PM
hallas added inline comments.
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()) ?

hallas marked an inline comment as done.May 6 2019, 5:35 PM

Please also update the commit message (the fix is now different).

Argh! I accidentally created a new diff instead: D21050 - I hope that is ok?

src/kitemviews/kstandarditem.cpp
48

I have removed this constructor instead since nobody uses it anyway :/

It's ok, just abandon this one ;)

hallas abandoned this revision.May 13 2019, 3:00 PM

This has been fixed in D21050