D17619: Unit test and fix for bug 401552
dfaure added a comment. What happened to the unittest promised by the commit log? ;) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17619 To: jtamate, dfaure, #frameworks Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns
D17619: Unit test and fix for bug 401552
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kcoredirlister.cpp:1580
> +while (!dir->lstItems.isEmpty()) {
> +const KFileItem oldItem = dir->lstItems.takeFirst();
> +KFileItem newItem = oldItem;
Why is this modifying lstItems? That's a somewhat costly operation, if it's not
needed. Why not a normal readonly iteration?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17619
To: jtamate, dfaure, #frameworks
Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns
D17619: Unit test and fix for bug 401552
jtamate updated this revision to Diff 47666. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. I think this time I got the problem right. One of the classics: I was modifying the list while it was being used. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17619?vs=47655&id=47666 REVISION DETAIL https://phabricator.kde.org/D17619 AFFECTED FILES src/core/kcoredirlister.cpp To: jtamate, dfaure, #frameworks Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns
D17619: Unit test and fix for bug 401552
elvisangelaccio added inline comments. INLINE COMMENTS > kdirlistertest.cpp:1299-1308 > +const QString dirX = m_tempDir.path() + "/x"; > +QVERIFY(QDir().mkdir(dirX)); > +const QString dirX1 = m_tempDir.path() + "/x/x1"; > +QVERIFY(QDir().mkdir(dirX1)); > +const QString dirX2 = m_tempDir.path() + "/x/x1/x2"; > +QVERIFY(QDir().mkdir(dirX2)); > +const QString dirX3 = m_tempDir.path() + "/x/x1/x2/x3"; I was not able to reproduce the crash with the steps you described in https://phabricator.kde.org/D10742#377375. I can semi-reliably reproduce it by renaming a non-empty folder expanded in the dolphin detail view. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17619 To: jtamate, dfaure, #frameworks Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns
D17619: Unit test and fix for bug 401552
elvisangelaccio added a comment. Unfortunately it's still crashing for me, even with this patch. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17619 To: jtamate, dfaure, #frameworks Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns
D17619: Unit test and fix for bug 401552
jtamate edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17619 To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D17619: Unit test and fix for bug 401552
jtamate created this revision. jtamate added reviewers: dfaure, Frameworks. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. jtamate requested review of this revision. REVISION SUMMARY Add a unit test that fails before applying the patch and doesn't after the patch (in skeleton only) Could someone that was able to reproduce the crash can test the patch to see if it really fixes the crash? TEST PLAN Fail in the unit test before applying the patch Doesn't fail after applying the patch. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17619 AFFECTED FILES autotests/kdirlistertest.cpp autotests/kdirlistertest.h src/core/kcoredirlister.cpp To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
