Re: kiowidgets_kdirmodeltest fail

[Albert Astals Cid]

El dimecres, 10 d’octubre de 2018, a les 16:33:43 CEST, Jaime va escriure:
> Hello,
> 
>   About
> https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/429/testReport/junit/(root)/TestSuite/kiowidgets_kdirmodeltest/
> 
>   I'm only able to reproduce the crash 1/7 times I run ctest -j3, never if
> I run the test alone or run make test.

FWIW I can reproduce the ASAN error quite easily with

while [ true ]; do ASAN_OPTIONS=detect_leaks=0 ./bin/kdirmodeltest 
testOverwriteFileWithDir; if [ $? -ne 0 ]; then break; fi; done

it doesn't go more than two or three loops.

> 
>   I've tried applying what worked for me in the 4th revision of
> https://phabricator.kde.org/D10742 to kdirmodel.cpp around line 600, but
> without success.
> 
>   Any idea how to fix it is welcomed.

From what i see, the problem is that you're storing QModelIndexes and using 
them later when the model has changed. 

This is not correct, you can only use a QModelIndex when you just got it or 
when it's passed it to you, if you need to store it you need to use 
QPersistentModelIndex.

I don't understand much the test, but i think the attached patch is a good fix, 
i.e. use the model indexes as soon as they happen and not store them. Also 
makes the test pass "forever" here.

Cheers,
  Albert

> 
> Best Regards.
> Jaime.
> 


diff --git a/autotests/kdirmodeltest.cpp b/autotests/kdirmodeltest.cpp
index 3c0ab0bb..c997ab55 100644
--- a/autotests/kdirmodeltest.cpp
+++ b/autotests/kdirmodeltest.cpp
@@ -1269,10 +1269,23 @@ void KDirModelTest::testOverwriteFileWithDir() // #151851 c4
     const QString file = path + "toplevelfile_1";
     const int oldTopLevelRowCount = m_dirModel->rowCount();
 
-    QSignalSpy spyRowsRemoved(m_dirModel, SIGNAL(rowsRemoved(QModelIndex,int,int)));
+    bool removalWithinTopLevel = false;
+    bool dataChangedAtFirstLevel = false;
+    connect(m_dirModel, &KDirModel::rowsRemoved, this, [&removalWithinTopLevel](const QModelIndex &index) {
+        if (!index.isValid()) {
+            // yes, that's what we have been waiting for
+            removalWithinTopLevel = true;
+        }
+    });
+    connect(m_dirModel, &KDirModel::dataChanged, this, [&dataChangedAtFirstLevel](const QModelIndex &index) {
+        if (index.isValid() && !index.parent().isValid()) {
+            // a change of a node whose parent is root, yay, that's it
+            dataChangedAtFirstLevel = true;
+        }
+    });
+
     checkedConnect(m_dirModel, SIGNAL(rowsRemoved(QModelIndex,int,int)),
                    &m_eventLoop, SLOT(exitLoop()));
-    QSignalSpy spyDataChanged(m_dirModel, SIGNAL(dataChanged(QModelIndex,QModelIndex)));
 
     KIO::Job *job = KIO::move(QUrl::fromLocalFile(dir), QUrl::fromLocalFile(file), KIO::HideProgressInfo);
     job->setUiDelegate(nullptr);
@@ -1285,24 +1298,9 @@ void KDirModelTest::testOverwriteFileWithDir() // #151851 c4
 
     // Wait for a removal within the top level (that's for the old file going away), and also
     // for a dataChanged which notifies us that a file has become a directory
-    bool removalWithinTopLevel = false;
-    bool dataChangedAtFirstLevel = false;
+
     int retries = 0;
     while ((!removalWithinTopLevel || !dataChangedAtFirstLevel) && retries < 100) {
-        for (int i = 0; i < spyRowsRemoved.size() && !removalWithinTopLevel; ++i) {
-            QModelIndex parent = spyRowsRemoved[i][0].value<QModelIndex>();
-            if (!parent.isValid()) {
-                // yes, that's what we have been waiting for
-                removalWithinTopLevel = true;
-            }
-        }
-        for (int i = 0; i < spyDataChanged.size() && !dataChangedAtFirstLevel; ++i) {
-            QModelIndex idx = spyDataChanged[i][0].value<QModelIndex>();
-            if (idx.isValid() && !idx.parent().isValid()) {
-                // a change of a node whose parent is root, yay, that's it
-                dataChangedAtFirstLevel = true;
-            }
-        }
         QTest::qWait(10);
         ++retries;
     }