File fix-kdirlister-forgetting-to-watch.patch of Package kdelibs4

Overview Repositories Revisions Requests Users Attributes Meta

File fix-kdirlister-forgetting-to-watch.patch of Package kdelibs4

From: David Faure <faure@kde.org>
Date: Thu, 29 Nov 2012 16:49:34 +0000
Subject: Fix KDirLister forgetting to watch a directory after listing it from the cache.
X-Git-Tag: v4.9.4
X-Git-Url: http://quickgit.kde.org/?p=kdelibs.git&a=commitdiff&h=75da8e8129f6bc152a781ff47fb8e741c65b584e
---
Fix KDirLister forgetting to watch a directory after listing it from the cache.

Many many thanks to Frank Reininghaus for the unittest that finally made
this issue reproduceable, based on feedback from users in the bug report.

The issue: whether to watch a directory with KDirWatch is refcounted.
Each lister showing the dir counts as one, the cache itself can add one,
and does so initially. If a dir is modified while it's only in the cache,
we mark it as dirty, stop watching, and we'll simply update the directory
when showing it to the user again later. At that point we need to start
watching it again. The old code would do decr+incr, I "optimized" this in
7b9cafaaf6af (oct 2010) to fix the bug that (with an initial refcount of 1),
decr would lead to 0 temporarily.
However if the item wasn't watched anymore (initial refcount of 0), the decr
would do nothing (if (c<0) c=0), and the incr would start the watching.
With 7b9cafaaf6af this all went away (I thought decr+incr==noop), so no watching.
The proper solution is obviously incr+decr_if_not_done_before_already
(when the cache stops watching the dir because a change happened).

BUG: 211472
FIXED-IN: 4.9.4
---

--- a/kio/kio/kdirlister.cpp
+++ b/kio/kio/kdirlister.cpp
@@ -172,18 +172,22 @@
 
         dirData.listersCurrentlyListing.append(lister);
 
-        DirItem *itemFromCache;
+        DirItem *itemFromCache = 0;
         if (itemU || (!_reload && (itemFromCache = itemsCached.take(urlStr)) ) ) {
             if (itemU) {
                 kDebug(7004) << "Entry already in use:" << _url;
                 // if _reload is set, then we'll emit cached items and then updateDirectory.
-                if (lister->d->autoUpdate)
-                    itemU->incAutoUpdate();
             } else {
                 kDebug(7004) << "Entry in cache:" << _url;
-                // In this code path, the itemsFromCache->decAutoUpdate + itemU->incAutoUpdate is optimized out
                 itemsInUse.insert(urlStr, itemFromCache);
                 itemU = itemFromCache;
+            }
+            if (lister->d->autoUpdate) {
+                itemU->incAutoUpdate();
+            }
+            if (itemFromCache && itemFromCache->watchedWhileInCache) {
+                itemFromCache->watchedWhileInCache = false;;
+                itemFromCache->decAutoUpdate();
             }
 
             emit lister->started(_url);
@@ -416,7 +420,7 @@
     Q_FOREACH(const KUrl& url, urls) {
         stopListingUrl(lister, url, silent);
     }
-    
+
 #if 0 // test code
     QHash<QString,KDirListerCacheDirectoryData>::iterator dirit = directoryData.begin();
     const QHash<QString,KDirListerCacheDirectoryData>::iterator dirend = directoryData.end();
@@ -618,9 +622,10 @@
                 kDebug(7004) << "Not adding a watch on " << item->url << " because it " <<
                     ( isManuallyMounted ? "is manually mounted" : "contains a manually mounted subdir" );
                 item->complete = false; // set to "dirty"
+            } else {
+                item->incAutoUpdate(); // keep watch
+                item->watchedWhileInCache = true;
             }
-            else
-                item->incAutoUpdate(); // keep watch
         }
         else
         {
@@ -746,7 +751,10 @@
     DirItem *item = itemsCached[_dir];
     if ( item && item->complete )
     {
+      // Stop watching items once they are only in the cache and not used anymore.
+      // We'll trigger an update when listing that dir again later.
       item->complete = false;
+      item->watchedWhileInCache = false;
       item->decAutoUpdate();
       // Hmm, this debug output might include login/password from the _dir URL.
       //kDebug(7004) << "directory " << _dir << " not in use, marked dirty.";

--- a/kio/kio/kdirlister_p.h
+++ b/kio/kio/kdirlister_p.h
@@ -319,6 +319,7 @@
     {
       autoUpdates = 0;
       complete = false;
+      watchedWhileInCache = false;
     }
 
     ~DirItem()
@@ -391,6 +392,9 @@
 
     // this directory is up-to-date
     bool complete;
+
+    // the directory is watched while being in the cache (useful for proper incAutoUpdate/decAutoUpdate count)
+    bool watchedWhileInCache;
 
     // the complete url of this directory
     KUrl url;

--- a/kio/tests/kdirlistertest.cpp
+++ b/kio/tests/kdirlistertest.cpp
@@ -871,6 +871,68 @@
     disconnect(&m_dirLister, 0, this, 0);
 }
 
+// A bug in the decAutoUpdate/incAutoUpdate logic made KDirLister stop watching a directory for changes,
+// and never watch it again when opening it from the cache.
+void KDirListerTest::testBug211472()
+{
+    m_items.clear();
+
+    KTempDir newDir;
+    const QString path = newDir.name() + "newsubdir/";
+    QDir().mkdir(path);
+    MyDirLister dirLister;
+    connect(&dirLister, SIGNAL(newItems(KFileItemList)), this, SLOT(slotNewItems(KFileItemList)));
+
+    dirLister.openUrl(KUrl(path));
+    QVERIFY(QTest::kWaitForSignal(&dirLister, SIGNAL(completed()), 1000));
+    QVERIFY(dirLister.isFinished());
+    QVERIFY(m_items.isEmpty());
+
+    if (true) {
+        // This block is required to trigger bug 211472.
+
+        // Go 'up' to the parent of 'newsubdir'.
+        dirLister.openUrl(KUrl(newDir.name()));
+        QVERIFY(QTest::kWaitForSignal(&dirLister, SIGNAL(completed()), 1000));
+        QVERIFY(dirLister.isFinished());
+        QVERIFY(!m_items.isEmpty());
+        m_items.clear();
+
+        // Create a file in 'newsubdir' while we are listing its parent dir.
+        createTestFile(path + "newFile-1");
+        // At this point, newsubdir is not used, so it's moved to the cache.
+        // This happens in checkUpdate, called when receiving a notification for the cached dir,
+        // this is why this unittest needs to create a test file in the subdir.
+        QTest::qWait(1000);
+        QVERIFY(m_items.isEmpty());
+
+        // Return to 'newsubdir'. It will be emitted from the cache, then an update will happen.
+        dirLister.openUrl(KUrl(path));
+        QVERIFY(QTest::kWaitForSignal(&dirLister, SIGNAL(completed()), 1000));
+        QVERIFY(QTest::kWaitForSignal(&dirLister, SIGNAL(completed()), 1000));
+        QVERIFY(dirLister.isFinished());
+        QCOMPARE(m_items.count(), 1);
+        m_items.clear();
+    }
+
+    // Now try to create a second file in 'newsubdir' and verify that the
+    // dir lister notices it.
+    QTest::qWait(1000); // We need a 1s timestamp difference on the dir, otherwise FAM won't notice anything.
+
+    createTestFile(path + "newFile-2");
+
+    int numTries = 0;
+    // Give time for KDirWatch to notify us
+    while (m_items.isEmpty()) {
+        QVERIFY(++numTries < 10);
+        QTest::qWait(200);
+    }
+    QCOMPARE(m_items.count(), 1);
+
+    newDir.unlink();
+    QVERIFY(QTest::kWaitForSignal(&dirLister, SIGNAL(clear()), 1000));
+}
+
 void KDirListerTest::testRedirection()
 {
     m_items.clear();
@@ -964,8 +1026,14 @@
     enterLoop();
     QCOMPARE(m_dirLister.spyClear.count(), 1);
     QCOMPARE(m_dirLister.spyClearKUrl.count(), 0);
-    QCOMPARE(m_dirLister.spyItemsDeleted.count(), 1);
-    QCOMPARE(m_dirLister.spyItemsDeleted[0][0].value<KFileItemList>().count(), 1);
+    KUrl::List deletedUrls;
+    for (int i = 0; i < m_dirLister.spyItemsDeleted.count(); ++i)
+        deletedUrls += m_dirLister.spyItemsDeleted[i][0].value<KFileItemList>().urlList();
+    //kDebug() << deletedUrls;
+    KUrl currentDirUrl = QUrl::fromLocalFile(path());
+    currentDirUrl.adjustPath(KUrl::RemoveTrailingSlash);
+    // Sometimes I get ("current/subdir", "current") here, but that seems ok.
+    QVERIFY(deletedUrls.contains(currentDirUrl));
 }
 
 int KDirListerTest::fileCount() const

--- a/kio/tests/kdirlistertest.h
+++ b/kio/tests/kdirlistertest.h
@@ -106,6 +106,7 @@
     void testOpenUrlTwice();
     void testOpenUrlTwiceWithKeep();
     void testOpenAndStop();
+    void testBug211472();
     void testRedirection();
     void testDeleteCurrentDir(); // must be last!

Places

File fix-kdirlister-forgetting-to-watch.patch of Package kdelibs4

Places