2 files changed, 149 insertions, 74 deletions

diff --git a/src/track/globaltrackcache.cpp b/src/track/globaltrackcache.cpp
index d608ed269e..1bb0cab342 100644
--- a/src/track/globaltrackcache.cpp
+++ b/src/track/globaltrackcache.cpp
@@ -36,18 +36,22 @@ TrackRef createTrackRef(const Track& track) {
 
 class EvictAndSaveFunctor {
   public:
-    explicit EvictAndSaveFunctor(GlobalTrackCacheEntryPointer cacheEntryPtr)
+    explicit EvictAndSaveFunctor(
+            GlobalTrackCacheEntryPointer cacheEntryPtr)
         : m_cacheEntryPtr(std::move(cacheEntryPtr)) {
     }
 
     void operator()(Track* plainPtr) {
         Q_UNUSED(plainPtr); // only used in DEBUG_ASSERT
+        DEBUG_ASSERT(m_cacheEntryPtr);
         DEBUG_ASSERT(plainPtr == m_cacheEntryPtr->getPlainPtr());
         // Here we move m_cacheEntryPtr and the owned track out of the
         // functor and the owning reference counting object.
         // This is required to break a cycle reference from the weak pointer
         // inside the cache entry to the same reference counting object.
         GlobalTrackCache::evictAndSaveCachedTrack(std::move(m_cacheEntryPtr));
+        // Verify that this functor is only invoked once
+        DEBUG_ASSERT(!m_cacheEntryPtr);
     }
 
     const GlobalTrackCacheEntryPointer& getCacheEntryPointer() const {
@@ -58,26 +62,6 @@ class EvictAndSaveFunctor {
     GlobalTrackCacheEntryPointer m_cacheEntryPtr;
 };
 
-
-void deleteTrack(Track* plainPtr) {
-    DEBUG_ASSERT(plainPtr);
-
-    // We safely delete the object via the Qt event queue instead
-    // of using operator delete! Otherwise the deleted track object
-    // might be accessed when processing cross-thread signals that
-    // are delayed within a queued connection and may arrive after
-    // the object has already been deleted.
-    if (traceLogEnabled()) {
-        plainPtr->dumpObjectInfo();
-    }
-    if (debugLogEnabled()) {
-        kLogger.debug()
-                << "Deleting"
-                << plainPtr;
-    }
-    plainPtr->deleteLater();
-}
-
 } // anonymous namespace
 
 GlobalTrackCacheLocker::GlobalTrackCacheLocker()
@@ -210,14 +194,21 @@ void GlobalTrackCacheResolver::initTrackIdAndUnlockCache(TrackId trackId) {
 }
 
 //static
-void GlobalTrackCache::createInstance(GlobalTrackCacheSaver* pDeleter) {
+void GlobalTrackCache::createInstance(
+        GlobalTrackCacheSaver* pSaver,
+        deleteTrackFn_t deleteTrackFn) {
     DEBUG_ASSERT(!s_pInstance);
-    s_pInstance = new GlobalTrackCache(pDeleter);
+    s_pInstance = new GlobalTrackCache(pSaver, deleteTrackFn);
 }
 
 //static
 void GlobalTrackCache::destroyInstance() {
     DEBUG_ASSERT(s_pInstance);
+    // Processing all pending events is required to evict all
+    // remaining references from the cache.
+    QCoreApplication::processEvents();
+    // Now the cache should be empty
+    DEBUG_ASSERT(GlobalTrackCacheLocker().isEmpty());
     GlobalTrackCache* pInstance = s_pInstance;
     // Reset the static/global pointer before entering the destructor
     s_pInstance = nullptr;
@@ -226,6 +217,32 @@ void GlobalTrackCache::destroyInstance() {
     pInstance->deleteLater();
 }
 
+void GlobalTrackCacheEntry::TrackDeleter::operator()(Track* pTrack) const {
+    DEBUG_ASSERT(pTrack);
+
+    // We safely delete the object via the Qt event queue instead
+    // of using operator delete! Otherwise the deleted track object
+    // might be accessed when processing cross-thread signals that
+    // are delayed within a queued connection and may arrive after
+    // the object has already been deleted.
+    if (traceLogEnabled()) {
+        pTrack->dumpObjectInfo();
+    }
+    if (debugLogEnabled()) {
+        kLogger.debug()
+                << "Deleting"
+                << pTrack;
+    }
+
+    if (m_deleteTrackFn) {
+        // Custom delete function
+        (*m_deleteTrackFn)(pTrack);
+    } else {
+        // Default delete function
+        pTrack->deleteLater();
+    }
+}
+
 //static
 void GlobalTrackCache::evictAndSaveCachedTrack(GlobalTrackCacheEntryPointer cacheEntryPtr) {
     // Any access to plainPtr before a validity check inside the
@@ -254,9 +271,12 @@ void GlobalTrackCache::evictAndSaveCachedTrack(GlobalTrackCacheEntryPointer cach
     }
 }
 
-GlobalTrackCache::GlobalTrackCache(GlobalTrackCacheSaver* pSaver)
+GlobalTrackCache::GlobalTrackCache(
+        GlobalTrackCacheSaver* pSaver,
+        deleteTrackFn_t deleteTrackFn)
     : m_mutex(QMutex::Recursive),
       m_pSaver(pSaver),
+      m_deleteTrackFn(deleteTrackFn),
       m_tracksById(kUnorderedCollectionMinCapacity, DbId::hash_fun) {
     DEBUG_ASSERT(m_pSaver);
     qRegisterMetaType<GlobalTrackCacheEntryPointer>("GlobalTrackCacheEntryPointer");
@@ -321,6 +341,18 @@ void GlobalTrackCache::relocateTracks(
     m_tracksByCanonicalLocation = std::move(relocatedTracksByCanonicalLocation);
 }
 
+void GlobalTrackCache::saveEvictedTrack(Track* pEvictedTrack) const {
+    DEBUG_ASSERT(pEvictedTrack);
+    // Disconnect all receivers and block signals before saving the
+    // track.
+    // NOTE(uklotzde, 2018-02-03): Simply disconnecting all receivers
+    // doesn't seem to work reliably. Emitting the clean() signal from
+    // a track that is about to deleted may cause access violations!!
+    pEvictedTrack->disconnect();
+    pEvictedTrack->blockSignals(true);
+    m_pSaver->saveEvictedTrack(pEvictedTrack);
+}
+
 void GlobalTrackCache::deactivate() {
     // Ideally the cache should be empty when destroyed.
     // But since this is difficult to achieve all remaining
@@ -331,7 +363,7 @@ void GlobalTrackCache::deactivate() {
     auto i = m_tracksById.begin();
     while (i != m_tracksById.end()) {
         Track* plainPtr= i->second->getPlainPtr();
-        m_pSaver->saveCachedTrack(plainPtr);
+        saveEvictedTrack(plainPtr);
         m_tracksByCanonicalLocation.erase(plainPtr->getCanonicalLocation());
         i = m_tracksById.erase(i);
     }
@@ -339,7 +371,7 @@ void GlobalTrackCache::deactivate() {
     auto j = m_tracksByCanonicalLocation.begin();
     while (j != m_tracksByCanonicalLocation.end()) {
         Track* plainPtr= j->second->getPlainPtr();
-        m_pSaver->saveCachedTrack(plainPtr);
+        saveEvictedTrack(plainPtr);
         j = m_tracksByCanonicalLocation.erase(j);
     }
 
@@ -413,13 +445,14 @@ TrackPointer GlobalTrackCache::lookupByRef(
 TrackPointer GlobalTrackCache::revive(
         GlobalTrackCacheEntryPointer entryPtr) {
 
-    TrackPointer savingPtr = entryPtr->getSavingWeakPtr().lock();
+    TrackPointer savingPtr = entryPtr->lock();
     if (savingPtr) {
         if (traceLogEnabled()) {
             kLogger.trace()
                     << "Found alive track"
                     << entryPtr->getPlainPtr();
         }
+        DEBUG_ASSERT(!savingPtr->signalsBlocked());
         return savingPtr;
     }
 
@@ -433,11 +466,12 @@ TrackPointer GlobalTrackCache::revive(
                 << "Reviving zombie track"
                 << entryPtr->getPlainPtr();
     }
-    DEBUG_ASSERT(entryPtr->getSavingWeakPtr().expired());
+    DEBUG_ASSERT(entryPtr->expired());
 
     savingPtr = TrackPointer(entryPtr->getPlainPtr(),
             EvictAndSaveFunctor(entryPtr));
-    entryPtr->setSavingWeakPtr(savingPtr);
+    entryPtr->init(savingPtr);
+    DEBUG_ASSERT(!savingPtr->signalsBlocked());
     return savingPtr;
 }
 
@@ -510,25 +544,19 @@ void GlobalTrackCache::resolve(
                 << "Cache miss - allocating track"
                 << trackRef;
     }
-    auto deletingPtr = std::unique_ptr<Track, void (&)(Track*)>(
+    auto deletingPtr = std::unique_ptr<Track, GlobalTrackCacheEntry::TrackDeleter>(
             new Track(
                     std::move(fileInfo),
                     std::move(pSecurityToken),
                     std::move(trackId)),
-            deleteTrack);
-    // Track objects live together with the cache on the main thread
-    // and will be deleted later within the event loop. But this
-    // function might be called from any thread, even from worker
-    // threads without an event loop. We need to move the newly
-    // created object to the target thread.
-    deletingPtr->moveToThread(QApplication::instance()->thread());
+            GlobalTrackCacheEntry::TrackDeleter(m_deleteTrackFn));
 
     auto cacheEntryPtr = std::make_shared<GlobalTrackCacheEntry>(
             std::move(deletingPtr));
     auto savingPtr = TrackPointer(
             cacheEntryPtr->getPlainPtr(),
             EvictAndSaveFunctor(cacheEntryPtr));
-    cacheEntryPtr->setSavingWeakPtr(savingPtr);
+    cacheEntryPtr->init(savingPtr);
 
     if (debugLogEnabled()) {
         kLogger.debug()
@@ -553,6 +581,14 @@ void GlobalTrackCache::resolve(
                 trackRef.getCanonicalLocation(),
                 cacheEntryPtr));
     }
+
+    // Track objects live together with the cache on the main thread
+    // and will be deleted later within the event loop. But this
+    // function might be called from any thread, even from worker
+    // threads without an event loop. We need to move the newly
+    // created object to the main thread.
+    savingPtr->moveToThread(QApplication::instance()->thread());
+
     pCacheResolver->initLookupResult(
             GlobalTrackCacheLookupResult::MISS,
             std::move(savingPtr),
@@ -608,7 +644,7 @@ void GlobalTrackCache::evictAndSave(
 
     GlobalTrackCacheLocker cacheLocker;
 
-    if (!cacheEntryPtr->getSavingWeakPtr().expired()) {
+    if (!cacheEntryPtr->expired()) {
         // We have handed out (revived) this track again after our reference count
         // drops to zero and before acquiring the lock at the beginning of this function
         if (debugLogEnabled()) {
@@ -619,31 +655,36 @@ void GlobalTrackCache::evictAndSave(
         return;
     }
 
-    if (!evict(cacheEntryPtr->getPlainPtr())) {
+    if (!tryEvict(cacheEntryPtr->getPlainPtr())) {
         // A second deleter has already evicted the track from cache after our
         // reference count drops to zero and before acquiring the lock at the
         // beginning of this function
         if (debugLogEnabled()) {
             kLogger.debug()
-                    << "Skip to save an already evicted track"
+                    << "Skip to save an already evicted track again"
                     << cacheEntryPtr->getPlainPtr();
         }
         return;
     }
 
-    DEBUG_ASSERT(isEvicted(cacheEntryPtr->getPlainPtr()));
-    m_pSaver->saveCachedTrack(cacheEntryPtr->getPlainPtr());
+    DEBUG_ASSERT(!isCached(cacheEntryPtr->getPlainPtr()));
+    saveEvictedTrack(cacheEntryPtr->getPlainPtr());
 
-    // here the cacheEntryPtr goes out of scope, the cache is deleted
-    // including the owned track
+    // here the cacheEntryPtr goes out of scope, the cache entry is
+    // deleted including the owned track
 }
 
-bool GlobalTrackCache::evict(Track* plainPtr) {
+bool GlobalTrackCache::tryEvict(Track* plainPtr) {
     DEBUG_ASSERT(plainPtr);
     // Make the cached track object invisible to avoid reusing
     // it before starting to save it. This is achieved by
     // removing it from both cache indices.
+    // Due to expected race conditions pointers might be evicted
+    // multiple times. Therefore we need to check the stored
+    // pointers to avoid evicting a new cached track object instead
+    // of the given plainPtr!!
     bool evicted = false;
+    bool notEvicted = false;
     const auto trackRef = createTrackRef(*plainPtr);
     if (debugLogEnabled()) {
         kLogger.debug()
@@ -654,39 +695,43 @@ bool GlobalTrackCache::evict(Track* plainPtr) {
     if (trackRef.hasId()) {
         const auto trackById = m_tracksById.find(trackRef.getId());
         if (trackById != m_tracksById.end()) {
-            DEBUG_ASSERT(trackById->second->getPlainPtr() == plainPtr);
-            m_tracksById.erase(trackById);
-            evicted = true;
+            if (trackById->second->getPlainPtr() == plainPtr) {
+                m_tracksById.erase(trackById);
+                evicted = true;
+            } else {
+                notEvicted = true;
+            }
         }
     }
     if (trackRef.hasCanonicalLocation()) {
         const auto trackByCanonicalLocation(
                 m_tracksByCanonicalLocation.find(trackRef.getCanonicalLocation()));
         if (m_tracksByCanonicalLocation.end() != trackByCanonicalLocation) {
-            DEBUG_ASSERT(trackByCanonicalLocation->second->getPlainPtr() == plainPtr);
-            m_tracksByCanonicalLocation.erase(
-                    trackByCanonicalLocation);
-            evicted = true;
+            if (trackByCanonicalLocation->second->getPlainPtr() == plainPtr) {
+                m_tracksByCanonicalLocation.erase(
+                        trackByCanonicalLocation);
+                evicted = true;
+            } else {
+                notEvicted = true;
+            }
         }
     }
-    DEBUG_ASSERT(isEvicted(plainPtr));
-    // Don't erase the pointer from m_cachedTracks here, because
-    // this function is invoked from 2 different contexts. The
-    // caller is responsible for doing this. Until then the cache
-    // is inconsistent and verifyConsistency() is expected to fail.
+    DEBUG_ASSERT(!isCached(plainPtr));
+    Q_UNUSED(notEvicted); // only used in debug assertion
+    DEBUG_ASSERT(!(evicted && notEvicted));
     return evicted;
 }
 
-bool GlobalTrackCache::isEvicted(Track* plainPtr) const {
+bool GlobalTrackCache::isCached(Track* plainPtr) const {
     for (auto&& entry: m_tracksById) {
         if (entry.second->getPlainPtr() == plainPtr) {
-            return false;
+            return true;
         }
     }
     for (auto&& entry: m_tracksByCanonicalLocation) {
         if (entry.second->getPlainPtr() == plainPtr) {
-              return false;
+              return true;
         }
     }
-    return true;
+    return false;
 }
diff --git a/src/track/globaltrackcache.h b/src/track/globaltrackcache.h
index 87945d1ee9..4cabed9209 100644
--- a/src/track/globaltrackcache.h
+++ b/src/track/globaltrackcache.h
@@ -32,6 +32,8 @@ protected:
     virtual ~GlobalTrackCacheRelocator() {}
 };
 
+typedef void (*deleteTrackFn_t)(Track*);
+
 class GlobalTrackCacheEntry final {
     // We need to hold two shared pointers, the deletingPtr is
     // responsible for the lifetime of the Track object itself.
@@ -39,25 +41,44 @@ class GlobalTrackCacheEntry final {
     // is not longer referenced, the track is saved and evicted
     // from the cache.
   public:
+    class TrackDeleter {
+    public:
+        explicit TrackDeleter(deleteTrackFn_t deleteTrackFn = nullptr)
+                : m_deleteTrackFn(deleteTrackFn) {
+        }
+
+        void operator()(Track* pTrack) const;
+
+    private:
+        deleteTrackFn_t m_deleteTrackFn;
+    };
+
     explicit GlobalTrackCacheEntry(
-            std::unique_ptr<Track, void (&)(Track*)> deletingPtr)
+            std::unique_ptr<Track, TrackDeleter> deletingPtr)
         : m_deletingPtr(std::move(deletingPtr)) {
     }
-
     GlobalTrackCacheEntry(const GlobalTrackCacheEntry& other) = delete;
+    GlobalTrackCacheEntry(GlobalTrackCacheEntry&&) = default;
+
+    void init(TrackWeakPointer savingWeakPtr) {
+        // Uninitialized or expired
+        DEBUG_ASSERT(!m_savingWeakPtr.lock());
+        m_savingWeakPtr = std::move(savingWeakPtr);
+    }
 
     Track* getPlainPtr() const {
         return m_deletingPtr.get();
     }
-    const TrackWeakPointer& getSavingWeakPtr() const {
-        return m_savingWeakPtr;
+
+    TrackPointer lock() const {
+        return m_savingWeakPtr.lock();
     }
-    void setSavingWeakPtr(TrackWeakPointer savingWeakPtr) {
-        m_savingWeakPtr = std::move(savingWeakPtr);
+    bool expired() const {
+        return m_savingWeakPtr.expired();
     }
 
   private:
-    std::unique_ptr<Track, void (&)(Track*)> m_deletingPtr;
+    std::unique_ptr<Track, TrackDeleter> m_deletingPtr;
     TrackWeakPointer m_savingWeakPtr;
 };
 
@@ -155,7 +176,7 @@ private:
 class /*interface*/ GlobalTrackCacheSaver {
 private:
     friend class GlobalTrackCache;
-    virtual void saveCachedTrack(Track* pTrack) noexcept = 0;
+    virtual void saveEvictedTrack(Track* pEvictedTrack) noexcept = 0;
 
 protected:
     virtual ~GlobalTrackCacheSaver() {}
@@ -165,7 +186,10 @@ class GlobalTrackCache : public QObject {
     Q_OBJECT
 
 public:
-    static void createInstance(GlobalTrackCacheSaver* pDeleter);
+    static void createInstance(
+            GlobalTrackCacheSaver* pSaver,
+            // A custom deleter is only needed for tests without an event loop!
+            deleteTrackFn_t deleteTrackFn = nullptr);
     // NOTE(uklotzde, 2018-02-20): We decided not to destroy the singular
     // instance during shutdown, because we are not able to guarantee that
     // all track references have been released before. Instead the singular
@@ -184,7 +208,9 @@ private:
     friend class GlobalTrackCacheLocker;
     friend class GlobalTrackCacheResolver;
 
-    explicit GlobalTrackCache(GlobalTrackCacheSaver* pDeleter);
+    GlobalTrackCache(
+            GlobalTrackCacheSaver* pSaver,
+            deleteTrackFn_t deleteTrackFn);
     ~GlobalTrackCache();
 
     void relocateTracks(
@@ -210,18 +236,22 @@ private:
 
     void purgeTrackId(TrackId trackId);
 
-    bool evict(Track* plainPtr);
-    bool isEvicted(Track* plainPtr) const;
+    bool tryEvict(Track* plainPtr);
+    bool isCached(Track* plainPtr) const;
 
     bool isEmpty() const;
 
     void deactivate();
 
+    void saveEvictedTrack(Track* pEvictedTrack) const;
+
     // Managed by GlobalTrackCacheLocker
     mutable QMutex m_mutex;
 
     GlobalTrackCacheSaver* m_pSaver;
 
+    deleteTrackFn_t m_deleteTrackFn;
+
     // This caches the unsaved Tracks by ID
     typedef std::unordered_map<TrackId, GlobalTrackCacheEntryPointer, TrackId::hash_fun_t> TracksById;
     TracksById m_tracksById;