ITunesXMLImporterTest: Fix sporadic crash due to a not initalized ref…

…erence.

Use a function call with a null check instead.

src/library/itunes/itunesfeature.cpp

@@ -315,7 +315,7 @@ std::unique_ptr<ITunesImporter> ITunesFeature::makeImporter() {
     }
 #endif
     qDebug() << "Using ITunesXMLImporter to read iTunes library from " << m_dbfile;
-    return std::make_unique<ITunesXMLImporter>(this, m_dbfile, m_cancelImport, std::move(dao));
+    return std::make_unique<ITunesXMLImporter>(this, m_dbfile, std::move(dao));
 }
 
 // This method is executed in a separate thread

src/library/itunes/itunesfeature.h

@@ -31,6 +31,11 @@ class ITunesFeature : public BaseExternalLibraryFeature {
 
     TreeItemModel* sidebarModel() const override;
 
+    // This is called form the ITunesXMLImporter thread
+    bool isImportCanceled() {
+        return m_cancelImport.load();
+    }
+
   public slots:
     void activate() override;
     void activate(bool forceReload);

src/library/itunes/itunesxmlimporter.cpp

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "library/itunes/itunesdao.h"
+#include "library/itunes/itunesfeature.h"
 #include "library/itunes/itunesimporter.h"
 #include "library/itunes/ituneslocalhosttoken.h"
 #include "library/queryutil.h"
@@ -43,15 +44,14 @@ const QString kRemote = "Remote";
 
 } // anonymous namespace
 
-ITunesXMLImporter::ITunesXMLImporter(LibraryFeature* parentFeature,
+ITunesXMLImporter::ITunesXMLImporter(
+        ITunesFeature* pParentFeature,
         const QString& xmlFilePath,
-        const std::atomic<bool>& cancelImport,
         std::unique_ptr<ITunesDAO> dao)
-        : m_parentFeature(parentFeature),
+        : m_pParentFeature(pParentFeature),
           m_xmlFilePath(xmlFilePath),
           m_xmlFile(xmlFilePath),
           m_xml(&m_xmlFile),
-          m_cancelImport(cancelImport),
           m_dao(std::move(dao)) {
     // By default set m_mixxxItunesRoot and m_dbItunesRoot to strip out
     // file://localhost/ from the URL. When we load the user's iTunes XML
@@ -73,7 +73,7 @@ ITunesImport ITunesXMLImporter::importLibrary() {
         return iTunesImport;
     }
 
-    while (!m_xml.atEnd() && !m_cancelImport.load()) {
+    while (!m_xml.atEnd() && canceled()) {
         m_xml.readNext();
         if (m_xml.isStartElement()) {
             if (m_xml.name() == QLatin1String("key")) {
@@ -90,9 +90,9 @@ ITunesImport ITunesXMLImporter::importLibrary() {
                 } else if (key == "Playlists") {
                     parsePlaylists();
 
-                    // The parent feature may ne null during testing
-                    std::unique_ptr<TreeItem> pRootItem = m_parentFeature
-                            ? TreeItem::newRoot(m_parentFeature)
+                    // The parent feature may be null during testing
+                    std::unique_ptr<TreeItem> pRootItem = m_pParentFeature
+                            ? TreeItem::newRoot(m_pParentFeature)
                             : std::make_unique<TreeItem>();
                     m_dao->appendPlaylistTree(pRootItem.get());
 
@@ -213,7 +213,7 @@ void ITunesXMLImporter::parseTracks() {
     qDebug() << "Parse iTunes music collection";
 
     // read all sunsequent <dict> until we reach the closing ENTRY tag
-    while (!m_xml.atEnd() && !m_cancelImport.load()) {
+    while (!m_xml.atEnd() && !canceled()) {
         m_xml.readNext();
 
         if (m_xml.isStartElement()) {
@@ -397,7 +397,7 @@ void ITunesXMLImporter::parseTrack() {
 void ITunesXMLImporter::parsePlaylists() {
     qDebug() << "Parse iTunes playlists";
 
-    while (!m_xml.atEnd() && !m_cancelImport.load()) {
+    while (!m_xml.atEnd() && !canceled()) {
         m_xml.readNext();
         // We process and iterate the <dict> tags holding playlist summary information here
         if (m_xml.isStartElement() && m_xml.name() == kDict) {
@@ -436,7 +436,7 @@ void ITunesXMLImporter::parsePlaylist() {
     bool isPlaylistItemsStarted = false;
 
     // We process and iterate the <dict> tags holding playlist summary information here
-    while (!m_xml.atEnd() && !m_cancelImport.load()) {
+    while (!m_xml.atEnd() && !canceled()) {
         m_xml.readNext();
 
         if (m_xml.isStartElement()) {
@@ -541,3 +541,11 @@ void ITunesXMLImporter::parsePlaylist() {
         m_dao->importPlaylistRelation(parentId, playlist.id);
     }
 }
+
+bool ITunesXMLImporter::canceled() {
+    // The parent feature may be null during testing
+    if (m_pParentFeature) {
+        return m_pParentFeature->isImportCanceled();
+    }
+    return false;
+}

src/library/itunes/itunesxmlimporter.h

@@ -9,27 +9,26 @@
 #include "library/itunes/itunesdao.h"
 #include "library/itunes/itunesimporter.h"
 #include "library/itunes/itunespathmapping.h"
-#include "library/libraryfeature.h"
+
+class ITunesFeature;
 
 /// An importer that parses an iTunes XML library.
 class ITunesXMLImporter : public ITunesImporter {
   public:
-    ITunesXMLImporter(LibraryFeature* parentFeature,
+    ITunesXMLImporter(
+            ITunesFeature* pParentFeature,
             const QString& xmlFilePath,
-            const std::atomic<bool>& cancelImport,
             std::unique_ptr<ITunesDAO> dao);
 
     ITunesImport importLibrary() override;
 
   private:
-    LibraryFeature* m_parentFeature;
+    bool canceled();
+
+    ITunesFeature* m_pParentFeature;
     const QString m_xmlFilePath;
     QFile m_xmlFile;
     QXmlStreamReader m_xml;
-    // The values behind the references are owned by the parent `ITunesFeature`
-    // thus there is an implicit contract here that this `ITunesXMLImporter` cannot
-    // outlive the feature (which should not happen anyway, since importers are short-lived).
-    const std::atomic<bool>& m_cancelImport;
     std::unique_ptr<ITunesDAO> m_dao;
 
     ITunesPathMapping m_pathMapping;

src/test/itunesxmlimportertest.cpp

@@ -22,12 +22,9 @@ class ITunesXMLImporterTest : public MixxxTest {
             const QString& xmlFileName, std::unique_ptr<ITunesDAO> dao) {
         QString xmlFilePath = getITunesTestDir().absoluteFilePath(xmlFileName);
         auto importer = std::make_unique<ITunesXMLImporter>(
-                nullptr, xmlFilePath, cancelImport, std::move(dao));
+                nullptr, xmlFilePath, std::move(dao));
         return importer;
     }
-
-  private:
-    std::atomic<bool> cancelImport;
 };
 
 class MockITunesDAO : public ITunesDAO {