D23875: KCoreDirLister: fix crash when creating new folders from kfilewidget

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Sorry it took me a long time to review this. The commit didn't fully satisfy my need to understand the underlying problem, so I had to find the time to debug the issue (thanks for the unittest, it really helps with that).
  
  AFAICS here's what happens:
  
  1. We create folder1 and immediately move the view into it
  2. So by the time `itemsAddedInDirectory()` (the DBus notification) is called for the parent directory, there is no KDirLister showing that directory anymore. Therefore this directory isn't listed again, it's just marked as dirty in the cache, for the next time the user will go into it (see `checkUpdate` where it says "not in use, marked dirty"). That's the usual optimization to avoid re-listing directories that are not shown anymore.
  3. Therefore the parent directory doesn't actually have a child item for folder1
  4. We then process the KDirWatch notification (`processPendingUpdates`) for the parent directory, which updates the mtime of that directory item. OK.
  5. Steps 1-3 happen again the same way for `folder1/folder2`
  6. Then, just like step 4, we process the KDirWatch notification for `folder1`, but it can't find that item in the parent directory due to step 2 above. ASSERT.
  
  The heart of the issue is that `findByUrl` first looks into "child items of the parent dir" then, for subdirs, falls back to the "." of the subdir (so it actually works for `folder1`).
  On the other hand, `reinsert` only supports child items, not "." items.
  This design is certainly quite tricky because for subdirs, the same URL is normally represented those two different items (except in this unusual scenario which skipped creating a child item in the parent directory...).
  
  OK, so how to fix this. A KDirWatch notification for a directory is about updating the mtime and/or permissions of that directory. But that's already done in `handleFileDirty`, as shown by `bin/kdirlistertest testDirPermissionChange`. A permission change for an item we're not showing (folder1, while in folder2) also works (handleFileDirty ends up in checkUpdate which marks it as dirty; tested manually).
  So... overall we don't actually need to do a `processPendingUpdates` for a KDirWatch notification for a directory in cache. It was added in commit 9c1e5d56cdfb8 <https://phabricator.kde.org/R446:9c1e5d56cdfb844f49a41010d307882939971da1> but `testDirPermissionChange` actually passes without it (at least nowadays). In addition, adding pending updates in `updateDirectory` called by `processPendingUpdates` seems very unclean to me (cyclic layering).
  So my suggested fix is to actually remove some code :-)
  
  I'll post a modified patch with my suggested changes.
  
  Thanks for pushing me to debug this, it's .... not trivial.

INLINE COMMENTS

> kfilewidgettest.cpp:516
> +        //created and entered, kdirlister would hit an assert (in reinsert()), bug 408801
> +        const QUrl url = QUrl::fromLocalFile(QDir::tempPath()).adjusted(QUrl::StripTrailingSlash);
> +        const QString dir = url.toLocalFile();

This test fails when being run twice in a row (without the fix), or if /tmp/folder1 already exists for some unrelated reason.

I suggest using QTemporaryDir instead of QDir::tempPath, so that the deletion of the QTemporaryDir cleans up in all cases (success or failure).

I did it locally to debug this, here it is:

  QTemporaryDir tempDir;
  QVERIFY(tempDir.isValid());
  const QString dir = tempDir.path();
  const QUrl url = QUrl::fromLocalFile(dir); 

(and then you can remove the cleanup code at the end of the method)

> kfilewidgettest.cpp:522
> +        fw.show();
> +        fw.activateWindow();
> +

show() and activateWindow() are not needed for this unittest to demonstrate the failure.

I'm trying to reduce the number of stuff popping up when running ctest, since that's rather annoying if I'm doing something else at the same time :-)

> kfilewidgettest.cpp:533
> +        // check that KFileWidget didn't crash
> +        QVERIFY(QTest::qWaitForWindowActive(&fw));
> +

Not needed, I get the assert even without this line.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23875

To: ahmadsamir, #frameworks, dfaure
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190915/1274d9c3/attachment-0001.html>