Do not delete shared track pointers until expired/released

6 files changed, 60 insertions, 36 deletions

diff --git a/src/library/library.cpp b/src/library/library.cpp
index 8cdd1a02ed..409c4c2975 100644
--- a/src/library/library.cpp
+++ b/src/library/library.cpp
@@ -422,7 +422,9 @@ void Library::slotSetTrackTableRowHeight(int rowHeight) {
     emit(setTrackTableRowHeight(rowHeight));
 }
 
-void Library::deleteCachedTrack(Track* pTrack) noexcept {
+void Library::deleteCachedTrack(
+        Track* pTrack,
+        delete_fun_t deleteFn) noexcept {
     // It can produce dangerous signal loops if the track is still
     // sending signals while being saved! All references to this
     // track have been dropped at this point, so there is no need
@@ -461,13 +463,16 @@ void Library::deleteCachedTrack(Track* pTrack) noexcept {
             // depending on the thread from which this method has
             // been invoked!
             Qt::AutoConnection,
-            Q_ARG(Track*, pTrack));
+            Q_ARG(Track*, pTrack),
+            Q_ARG(GlobalTrackCacheDeleter::delete_fun_t, deleteFn));
 }
 
-void Library::saveAndDeleteTrack(Track* pTrack) {
+void Library::saveAndDeleteTrack(
+        Track* pTrack,
+        GlobalTrackCacheDeleter::delete_fun_t deleteFn) {
     // Update the database
     m_pTrackCollection->saveTrack(pTrack);
 
     // Finally schedule the track for deletion
-    GlobalTrackCache::deleteTrack(pTrack);
+    deleteFn(pTrack);
 }
diff --git a/src/library/library.h b/src/library/library.h
index 2edad48c2c..c702d57b15 100644
--- a/src/library/library.h
+++ b/src/library/library.h
@@ -81,7 +81,9 @@ class Library: public QObject,
 
     static const int kDefaultRowHeightPx;
 
-    void deleteCachedTrack(Track* pTrack) noexcept override;
+    void deleteCachedTrack(
+            Track* pTrack,
+            delete_fun_t deleteFn) noexcept override;
 
   public slots:
     void slotShowTrackModel(QAbstractItemModel* model);
@@ -125,7 +127,9 @@ class Library: public QObject,
     void scanFinished();
 
   private:
-    Q_INVOKABLE void saveAndDeleteTrack(Track* pTrack);
+    Q_INVOKABLE void saveAndDeleteTrack(
+            Track* pTrack,
+            GlobalTrackCacheDeleter::delete_fun_t deleteFn);
 
     const UserSettingsPointer m_pConfig;
 
diff --git a/src/test/globaltrackcache_test.cpp b/src/test/globaltrackcache_test.cpp
index ce20cfd827..f76fbc3dad 100644
--- a/src/test/globaltrackcache_test.cpp
+++ b/src/test/globaltrackcache_test.cpp
@@ -60,9 +60,11 @@ class TrackTitleThread: public QThread {
 
 class GlobalTrackCacheTest: public MixxxTest, public virtual GlobalTrackCacheDeleter {
   public:
-    void deleteCachedTrack(Track* pTrack) noexcept override {
+    void deleteCachedTrack(
+            Track* pTrack,
+            delete_fun_t deleteFn) noexcept override {
         ASSERT_FALSE(pTrack == nullptr);
-        GlobalTrackCache::deleteTrack(pTrack);
+        deleteFn(pTrack);
     }
 
   protected:
diff --git a/src/test/librarytest.h b/src/test/librarytest.h
index b5540e0d7c..b55a5e9d86 100644
--- a/src/test/librarytest.h
+++ b/src/test/librarytest.h
@@ -14,10 +14,12 @@ class LibraryTest : public MixxxTest,
     public virtual /*implements*/ GlobalTrackCacheDeleter {
 
   public:
-    void deleteCachedTrack(Track* pTrack) noexcept override {
+    void deleteCachedTrack(
+            Track* pTrack,
+            delete_fun_t deleteFn) noexcept override {
         m_trackCollection.exportTrackMetadata(pTrack);
         m_trackCollection.saveTrack(pTrack);
-        GlobalTrackCache::deleteTrack(pTrack);
+        deleteFn(pTrack);
     }
 
   protected:
diff --git a/src/track/globaltrackcache.cpp b/src/track/globaltrackcache.cpp
index 32362a713e..2366cdfabe 100644
--- a/src/track/globaltrackcache.cpp
+++ b/src/track/globaltrackcache.cpp
@@ -33,6 +33,36 @@ TrackRef createTrackRef(const Track& track) {
     return TrackRef::fromFileInfo(track.getFileInfo(), track.getId());
 }
 
+void finalDeleter(Track* plainPtr) noexcept {
+    VERIFY_OR_DEBUG_ASSERT(plainPtr) {
+        kLogger.warning()
+                << "Cannot delete null track pointer";
+        return;
+    }
+    if (traceLogEnabled()) {
+        plainPtr->dumpObjectInfo();
+    }
+    // Track object must not be deleted by operator new!
+    // 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 (debugLogEnabled()) {
+        kLogger.debug()
+                << "Deleting track"
+                << plainPtr;
+    }
+    plainPtr->deleteLater();
+}
+
+void unexpiredDeleter(Track* plainPtr) noexcept {
+    if (debugLogEnabled()) {
+        kLogger.debug()
+                << "Skipping deletion of track"
+                << plainPtr;
+    }
+}
+
 } // anonymous namespace
 
 GlobalTrackCacheLocker::GlobalTrackCacheLocker()
@@ -178,28 +208,6 @@ void GlobalTrackCache::destroyInstance() {
     delete pInstance;
 }
 
-void GlobalTrackCache::deleteTrack(Track* plainPtr) {
-    VERIFY_OR_DEBUG_ASSERT(plainPtr) {
-        kLogger.warning()
-                << "Cannot delete null track pointer";
-        return;
-    }
-    if (traceLogEnabled()) {
-        plainPtr->dumpObjectInfo();
-    }
-    // Track object must not be deleted by operator new!
-    // 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 (debugLogEnabled()) {
-        kLogger.debug()
-                << "Deleting track"
-                << plainPtr;
-    }
-    plainPtr->deleteLater();
-}
-
 //static
 void GlobalTrackCache::deleter(Track* plainPtr) {
     DEBUG_ASSERT(plainPtr);
@@ -684,7 +692,9 @@ bool GlobalTrackCache::evictAndDelete(
         // The evicted entry must not be accessible anymore!
         DEBUG_ASSERT(m_indexedTracks.find(plainPtr) == m_indexedTracks.end());
         DEBUG_ASSERT(!lookupByRef(trackRef));
-        m_pDeleter->deleteCachedTrack(plainPtr);
+        m_pDeleter->deleteCachedTrack(
+                plainPtr,
+                evictUnexpired ? unexpiredDeleter : finalDeleter);
         return true;
     } else {
         // ...otherwise the given plainPtr is still referenced within
diff --git a/src/track/globaltrackcache.h b/src/track/globaltrackcache.h
index 811622dcc7..be70052298 100644
--- a/src/track/globaltrackcache.h
+++ b/src/track/globaltrackcache.h
@@ -134,8 +134,11 @@ private:
 // the callee.
 class /*interface*/ GlobalTrackCacheDeleter {
 public:
+    typedef void (*delete_fun_t)(Track*);
+
     virtual void deleteCachedTrack(
-            Track* /*not null*/ plainPtr) noexcept = 0;
+            Track* /*not null*/ plainPtr,
+            delete_fun_t deleterFn) noexcept = 0;
 
 protected:
     virtual ~GlobalTrackCacheDeleter() {}
@@ -152,8 +155,6 @@ public:
     // See also: GlobalTrackCacheLocker::deactivateCache()
     static void destroyInstance();
 
-    static void deleteTrack(Track* plainPtr);
-
 private:
     friend class GlobalTrackCacheLocker;
     friend class GlobalTrackCacheResolver;