File Fix-crash-during-musicbrainz-search.patch of Package amarok

From 88ee85a8333cf54dc123fef5e07a1c92ab9f7261 Mon Sep 17 00:00:00 2001
From: Sergey Ivanov <123kash@gmail.com>
Date: Sun, 20 Aug 2017 11:25:57 +0300
Subject: Fix crash during musicbrainz search.

BUG: 328359
REVIEW: 130232
---
 ChangeLog                                |   3 +-
 src/musicbrainz/MusicBrainzFinder.cpp    |   6 +-
 src/musicbrainz/MusicBrainzFinder.h      |   3 +-
 src/musicbrainz/MusicBrainzTagsItem.cpp  | 188 +++++++++++--------------------
 src/musicbrainz/MusicBrainzTagsItem.h    |   6 +-
 src/musicbrainz/MusicBrainzTagsModel.cpp |  66 +++++++++--
 src/musicbrainz/MusicBrainzTagsModel.h   |   2 +
 src/musicbrainz/MusicBrainzXmlParser.cpp |   3 +-
 src/musicbrainz/MusicBrainzXmlParser.h   |   2 +-
 9 files changed, 141 insertions(+), 138 deletions(-)

diff --git a/src/musicbrainz/MusicBrainzFinder.cpp b/src/musicbrainz/MusicBrainzFinder.cpp
index 9a1b3bb..2598a0b 100644
--- a/src/musicbrainz/MusicBrainzFinder.cpp
+++ b/src/musicbrainz/MusicBrainzFinder.cpp
@@ -132,8 +132,7 @@ MusicBrainzFinder::sendNewRequest()
 }
 
 void
-MusicBrainzFinder::gotAuthenticationRequest( const QNetworkReply *reply,
-                                             QAuthenticator *authenticator ) const
+MusicBrainzFinder::gotAuthenticationRequest( const QNetworkReply *reply, QAuthenticator *authenticator )
 {
     if( reply->url().host() == mb_host )
     {
@@ -644,8 +643,9 @@ MusicBrainzFinder::compileRequest( QUrl &url )
     url.setPort( mb_port );
 
     QNetworkRequest req( url );
-    req.setRawHeader( "User-Agent" , "Amarok" );
+    req.setRawHeader( "Accept", "application/xml");
     req.setRawHeader( "Connection", "Keep-Alive" );
+    req.setRawHeader( "User-Agent" , "Amarok" );
 
     if( !m_timer->isActive() )
         m_timer->start();
diff --git a/src/musicbrainz/MusicBrainzFinder.h b/src/musicbrainz/MusicBrainzFinder.h
index beb1551..4dbd4a5 100644
--- a/src/musicbrainz/MusicBrainzFinder.h
+++ b/src/musicbrainz/MusicBrainzFinder.h
@@ -51,8 +51,7 @@ class MusicBrainzFinder : public QObject
 
     private slots:
         void sendNewRequest();
-        void gotAuthenticationRequest( const QNetworkReply *reply,
-                                       QAuthenticator *authenticator ) const;
+        void gotAuthenticationRequest( const QNetworkReply *reply, QAuthenticator *authenticator );
         void gotReplyError( QNetworkReply::NetworkError code );
         void gotReply( QNetworkReply *reply );
 
diff --git a/src/musicbrainz/MusicBrainzTagsItem.cpp b/src/musicbrainz/MusicBrainzTagsItem.cpp
index 0941a90..6430392 100644
--- a/src/musicbrainz/MusicBrainzTagsItem.cpp
+++ b/src/musicbrainz/MusicBrainzTagsItem.cpp
@@ -65,146 +65,75 @@ MusicBrainzTagsItem::child( const int row ) const
 void
 MusicBrainzTagsItem::appendChild( MusicBrainzTagsItem *newItem )
 {
-    DEBUG_BLOCK
+    QWriteLocker lock( &m_childrenLock );
+    m_childItems.append( newItem );
+    newItem->setParent( this );
 
-    if( newItem->track().isNull() )
+    if( !newItem->data().isEmpty() )
     {
-        delete newItem;
-        return;
-    }
+        newItem->recalcSimilarityRate();
 
-    if( m_track.isNull() )
-    {
-        // Root item.
-        bool found = false;
+#define MAKE_DATA_LIST( k ) { QVariantList list; if( newItem->dataContains( k ) ) list.append( newItem->dataValue( k ) ); newItem->dataInsert( k, list ); }
 
-        /*
-         * A write lock is required, because with a read lock two or more threads
-         * referencing the same track could search for a matching item at the same time,
-         * fail, and queue up to create a new one, thus resulting in duplicates.
-         */
-        QWriteLocker lock( &m_childrenLock );
-        foreach( MusicBrainzTagsItem *item, m_childItems )
-        {
-            if( item->track() == newItem->track() )
-            {
-                found = true;
-                if( !newItem->data().isEmpty() )
-                    item->appendChild( newItem );
-                break;
-            }
-        }
+        MAKE_DATA_LIST( MusicBrainz::TRACKID );
+        MAKE_DATA_LIST( MusicBrainz::ARTISTID );
+        MAKE_DATA_LIST( MusicBrainz::RELEASEID );
 
-        if( !found )
-        {
-            MusicBrainzTagsItem *newChild = new MusicBrainzTagsItem( this, newItem->track() );
-            if( !newItem->data().isEmpty() )
-                newChild->appendChild( newItem );
-            m_childItems.append( newChild );
-        }
-    }
-    else
-    {
-        if( m_track != newItem->track() )
-        {
-            debug() << "Trying to insert track data to the wrong tree branch.";
-            delete newItem;
-            return;
-        }
-
-        bool found = false;
-        newItem->setParent( this );
-
-        // Already locked in parent call (the same logic applies).
-        foreach( MusicBrainzTagsItem *item, m_childItems )
-        {
-            if( newItem == item )
-            {
-                found = true;
-                // Merge the two matching results.
-                debug() << "Track" << newItem->dataValue( MusicBrainz::TRACKID ).toString() << "already in the tree.";
-                item->mergeWith( newItem );
-                delete newItem;
-                break;
-            }
-        }
-
-        if( !found )
-        {
-            newItem->dataInsert( MusicBrainz::SIMILARITY,
-                                 newItem->dataValue( MusicBrainz::MUSICBRAINZ ).toFloat() +
-                                 newItem->dataValue( MusicBrainz::MUSICDNS ).toFloat() );
-
-            QVariantList trackList;
-            QVariantList artistList;
-            QVariantList releaseList;
-            if( newItem->dataContains( MusicBrainz::TRACKID ) )
-                trackList.append( newItem->dataValue( MusicBrainz::TRACKID ) );
-            if( newItem->dataContains( MusicBrainz::ARTISTID ) )
-                artistList.append( newItem->dataValue( MusicBrainz::ARTISTID ) );
-            if( newItem->dataContains( MusicBrainz::RELEASEID ) )
-                releaseList.append( newItem->dataValue( MusicBrainz::RELEASEID ) );
-            newItem->dataInsert( MusicBrainz::TRACKID, trackList );
-            newItem->dataInsert( MusicBrainz::ARTISTID, artistList );
-            newItem->dataInsert( MusicBrainz::RELEASEID, releaseList );
-
-            m_childItems.append( newItem );
-        }
+#undef MAKE_DATA_LIST
     }
 }
 
 void
-MusicBrainzTagsItem::mergeWith( MusicBrainzTagsItem *item )
+MusicBrainzTagsItem::mergeData( const QVariantMap &tags )
 {
-    /*
-     * The lock is inherited from appendChild(). This method is not supposed to be called
-     * elsewhere.
-     */
+    if( tags.isEmpty() )
+        return;
 
+    MusicBrainzTagsItem fakeItem( this, m_track, tags );
     // Calculate the future score of the result when merged.
-    if( !item->dataContains( MusicBrainz::MUSICBRAINZ ) &&
-        dataContains( MusicBrainz::MUSICBRAINZ ) )
-        item->dataInsert( MusicBrainz::MUSICBRAINZ,
-                          dataValue( MusicBrainz::MUSICBRAINZ ) );
-    if( !item->dataContains( MusicBrainz::MUSICDNS ) &&
-        dataContains( MusicBrainz::MUSICDNS ) )
-        item->dataInsert( MusicBrainz::MUSICDNS,
-                          dataValue( MusicBrainz::MUSICDNS ) );
-    item->dataInsert( MusicBrainz::SIMILARITY,
-                      item->dataValue( MusicBrainz::MUSICBRAINZ ).toFloat() +
-                      item->dataValue( MusicBrainz::MUSICDNS ).toFloat() );
+    if( !fakeItem.dataContains( MusicBrainz::MUSICBRAINZ ) && dataContains( MusicBrainz::MUSICBRAINZ ) )
+        fakeItem.dataInsert( MusicBrainz::MUSICBRAINZ, dataValue( MusicBrainz::MUSICBRAINZ ) );
+
+    if( !fakeItem.dataContains( MusicBrainz::MUSICDNS ) && dataContains( MusicBrainz::MUSICDNS ) )
+        fakeItem.dataInsert( MusicBrainz::MUSICDNS, dataValue( MusicBrainz::MUSICDNS ) );
+
+    fakeItem.recalcSimilarityRate();
 
     QVariantList trackList = dataValue( MusicBrainz::TRACKID ).toList();
     QVariantList artistList = dataValue( MusicBrainz::ARTISTID ).toList();
     QVariantList releaseList = dataValue( MusicBrainz::RELEASEID ).toList();
-    if( item->score() > score() )
+    if( fakeItem.score() > score() )
     {
         // Update the score.
-        if( item->dataContains( MusicBrainz::MUSICBRAINZ ) )
-            dataInsert( MusicBrainz::MUSICBRAINZ,
-                        item->dataValue( MusicBrainz::MUSICBRAINZ ) );
-        if( item->dataContains( MusicBrainz::MUSICDNS ) )
-            dataInsert( MusicBrainz::MUSICDNS,
-                        item->dataValue( MusicBrainz::MUSICDNS ) );
-        dataInsert( MusicBrainz::SIMILARITY,
-                    item->dataValue( MusicBrainz::SIMILARITY ) );
-
-        if( item->dataContains( MusicBrainz::TRACKID ) )
-            trackList.prepend( item->dataValue( MusicBrainz::TRACKID ) );
-        if( item->dataContains( MusicBrainz::ARTISTID ) )
-            artistList.prepend( item->dataValue( MusicBrainz::ARTISTID ) );
-        if( item->dataContains( MusicBrainz::RELEASEID ) )
-            releaseList.prepend( item->dataValue( MusicBrainz::RELEASEID ) );
+        if( fakeItem.dataContains( MusicBrainz::MUSICBRAINZ ) )
+            dataInsert( MusicBrainz::MUSICBRAINZ, fakeItem.dataValue( MusicBrainz::MUSICBRAINZ ) );
+
+        if( fakeItem.dataContains( MusicBrainz::MUSICDNS ) )
+            dataInsert( MusicBrainz::MUSICDNS, fakeItem.dataValue( MusicBrainz::MUSICDNS ) );
+
+        recalcSimilarityRate();
+
+        if( fakeItem.dataContains( MusicBrainz::TRACKID ) )
+            trackList.prepend( fakeItem.dataValue( MusicBrainz::TRACKID ) );
+
+        if( fakeItem.dataContains( MusicBrainz::ARTISTID ) )
+            artistList.prepend( fakeItem.dataValue( MusicBrainz::ARTISTID ) );
+
+        if( fakeItem.dataContains( MusicBrainz::RELEASEID ) )
+            releaseList.prepend( fakeItem.dataValue( MusicBrainz::RELEASEID ) );
     }
     else
     {
-        if( item->dataContains( MusicBrainz::TRACKID ) )
-            trackList.append( item->dataValue( MusicBrainz::TRACKID ) );
-        if( item->dataContains( MusicBrainz::ARTISTID ) )
-            artistList.append( item->dataValue( MusicBrainz::ARTISTID ) );
-        if( item->dataContains( MusicBrainz::RELEASEID ) )
-            releaseList.append( item->dataValue( MusicBrainz::RELEASEID ) );
+        if( fakeItem.dataContains( MusicBrainz::TRACKID ) )
+            trackList.append( fakeItem.dataValue( MusicBrainz::TRACKID ) );
+
+        if( fakeItem.dataContains( MusicBrainz::ARTISTID ) )
+            artistList.append( fakeItem.dataValue( MusicBrainz::ARTISTID ) );
+
+        if( fakeItem.dataContains( MusicBrainz::RELEASEID ) )
+            releaseList.append( fakeItem.dataValue( MusicBrainz::RELEASEID ) );
     }
+
     dataInsert( MusicBrainz::TRACKID, trackList );
     dataInsert( MusicBrainz::ARTISTID, artistList );
     dataInsert( MusicBrainz::RELEASEID, releaseList );
@@ -476,10 +405,11 @@ MusicBrainzTagsItem::clearChoices()
 }
 
 bool
-MusicBrainzTagsItem::operator==( const MusicBrainzTagsItem* item ) const
+MusicBrainzTagsItem::isSimilar( const QVariantMap &tags ) const
 {
     QReadLocker lock( &m_dataLock );
-#define MATCH( k, t ) dataValue( k ).t() == item->dataValue( k ).t()
+    QVariant empty;
+#define MATCH( k, t ) ( dataValue( k ).t() == ( tags.contains( k ) ? tags.value( k ) : empty ).t() )
     /*
      * This is the information shown to the user: he will never be able to
      * distinguish between two tracks with the same information.
@@ -494,3 +424,21 @@ MusicBrainzTagsItem::operator==( const MusicBrainzTagsItem* item ) const
            MATCH( Meta::Field::TRACKNUMBER, toInt );
 #undef MATCH
 }
+
+bool
+MusicBrainzTagsItem::operator==( const MusicBrainzTagsItem* item ) const
+{
+    return isSimilar( item->data() );
+}
+
+bool
+MusicBrainzTagsItem::operator==( const Meta::TrackPtr &track) const
+{
+    return m_track == track;
+}
+
+void
+MusicBrainzTagsItem::recalcSimilarityRate()
+{
+    dataInsert( MusicBrainz::SIMILARITY, dataValue( MusicBrainz::MUSICBRAINZ ).toFloat() + dataValue( MusicBrainz::MUSICDNS ).toFloat() );
+}
diff --git a/src/musicbrainz/MusicBrainzTagsItem.h b/src/musicbrainz/MusicBrainzTagsItem.h
index 6852c9f..11efb52 100644
--- a/src/musicbrainz/MusicBrainzTagsItem.h
+++ b/src/musicbrainz/MusicBrainzTagsItem.h
@@ -42,6 +42,7 @@ class MusicBrainzTagsItem
         QVariantMap data() const;
         QVariant data( const int column ) const;
         void setData( const QVariantMap &tags );
+        void mergeData( const QVariantMap &tags );
         bool dataContains( const QString &key ) const;
         QVariant dataValue( const QString &key ) const;
 
@@ -52,12 +53,15 @@ class MusicBrainzTagsItem
         bool chooseBestMatchFromRelease( const QStringList &releases );
         void clearChoices();
 
+        bool isSimilar( const QVariantMap &tags ) const;
+
         bool operator==( const MusicBrainzTagsItem *item ) const;
+        bool operator==( const Meta::TrackPtr &track) const;
 
     private:
         void setParent( MusicBrainzTagsItem *parent );
-        void mergeWith( MusicBrainzTagsItem *item );
         void dataInsert( const QString &key, const QVariant &value );
+        void recalcSimilarityRate();
 
         MusicBrainzTagsItem *m_parent;
         QList<MusicBrainzTagsItem *> m_childItems;
diff --git a/src/musicbrainz/MusicBrainzTagsModel.cpp b/src/musicbrainz/MusicBrainzTagsModel.cpp
index 0ffb284..e7bde4c 100644
--- a/src/musicbrainz/MusicBrainzTagsModel.cpp
+++ b/src/musicbrainz/MusicBrainzTagsModel.cpp
@@ -204,7 +204,10 @@ MusicBrainzTagsModel::setData( const QModelIndex &index, const QVariant &value,
 Qt::ItemFlags
 MusicBrainzTagsModel::flags( const QModelIndex &index ) const
 {
-    if( !index.isValid() || !parent( index ).isValid() )
+    if( !index.isValid() )
+        return QAbstractItemModel::flags( index );
+
+    if( !parent( index ).isValid() )
         // Disable items with no children.
         return QAbstractItemModel::flags( index ) ^
                ( ( !static_cast<MusicBrainzTagsItem *>( index.internalPointer() )->childCount() )?
@@ -248,22 +251,67 @@ MusicBrainzTagsModel::columnCount( const QModelIndex &parent ) const
 void
 MusicBrainzTagsModel::addTrack( const Meta::TrackPtr track, const QVariantMap tags )
 {
-    QModelIndex parent;
-    int row = rowCount();
-    for( int i = 0; i < m_rootItem->childCount(); i++ )
+    DEBUG_BLOCK
+
+    if( track.isNull() )
+        return;
+
+    QMutexLocker lock( &m_modelLock );
+
+    MusicBrainzTagsItem *trackItem = NULL;
+    QModelIndex trackIndex;
+    for( int i = 0; i < m_rootItem->childCount(); ++i )
     {
         MusicBrainzTagsItem *item = m_rootItem->child( i );
         if( track == item->track() )
         {
-            parent = index( i, 0 );
-            row = rowCount( parent );
+            trackItem = item;
+            trackIndex = index( i, 0 );
+
+            break;
+        }
+    }
+
+    if( !trackItem )
+    {
+        trackItem = new MusicBrainzTagsItem( m_rootItem, track );
+
+        beginInsertRows( QModelIndex(), m_rootItem->childCount(), m_rootItem->childCount() );
+        m_rootItem->appendChild( trackItem );
+        endInsertRows();
+
+        trackIndex = index( m_rootItem->childCount() - 1, 0 );
+    }
+
+    if( tags.isEmpty() )
+    {
+        warning() << "Search result contains no data for track: " << track->prettyName();
+        return;
+    }
+
+    MusicBrainzTagsItem *similarItem = NULL;
+    for( int i = 0; i < trackItem->childCount(); ++i )
+    {
+        MusicBrainzTagsItem *item = trackItem->child( i );
+        if( item->isSimilar( tags ) )
+        {
+            similarItem = item;
+
+            item->mergeData( tags );
+            emit dataChanged( index( i, 0, trackIndex ), index(i, columnCount() - 1, trackIndex ) );
+
             break;
         }
     }
 
-    beginInsertRows( parent, row, row );
-    m_rootItem->appendChild( new MusicBrainzTagsItem( m_rootItem, track, tags ) );
-    endInsertRows();
+    if( !similarItem )
+    {
+        MusicBrainzTagsItem *item = new MusicBrainzTagsItem( trackItem, track, tags );
+
+        beginInsertRows( trackIndex, trackItem->childCount(), trackItem->childCount() );
+        trackItem->appendChild( item );
+        endInsertRows();
+    }
 }
 
 QMap<Meta::TrackPtr, QVariantMap>
diff --git a/src/musicbrainz/MusicBrainzTagsModel.h b/src/musicbrainz/MusicBrainzTagsModel.h
index eaee7f8..3ea2fd2 100644
--- a/src/musicbrainz/MusicBrainzTagsModel.h
+++ b/src/musicbrainz/MusicBrainzTagsModel.h
@@ -21,6 +21,7 @@
 #include "core/meta/forward_declarations.h"
 
 #include <QAbstractItemModel>
+#include <QMutex>
 
 class MusicBrainzTagsItem;
 
@@ -69,6 +70,7 @@ class MusicBrainzTagsModel : public QAbstractItemModel
 
     private:
         MusicBrainzTagsItem *m_rootItem;
+        mutable QMutex m_modelLock;
 };
 
 #endif // MUSICBRAINZTAGSMDOEL_H
diff --git a/src/musicbrainz/MusicBrainzXmlParser.cpp b/src/musicbrainz/MusicBrainzXmlParser.cpp
index 290a275..40d36a3 100644
--- a/src/musicbrainz/MusicBrainzXmlParser.cpp
+++ b/src/musicbrainz/MusicBrainzXmlParser.cpp
@@ -26,7 +26,7 @@
 #include <QStringList>
 #include <QVariantList>
 
-MusicBrainzXmlParser::MusicBrainzXmlParser( QString &doc )
+MusicBrainzXmlParser::MusicBrainzXmlParser( const QString &doc )
     : ThreadWeaver::Job()
     , m_doc( "musicbrainz" )
     , m_type( 0 )
@@ -38,6 +38,7 @@ void
 MusicBrainzXmlParser::run()
 {
     DEBUG_BLOCK
+
     QDomElement docElem = m_doc.documentElement();
     parseElement( docElem );
 }
diff --git a/src/musicbrainz/MusicBrainzXmlParser.h b/src/musicbrainz/MusicBrainzXmlParser.h
index c7f9e72..7b2876b 100644
--- a/src/musicbrainz/MusicBrainzXmlParser.h
+++ b/src/musicbrainz/MusicBrainzXmlParser.h
@@ -34,7 +34,7 @@ class MusicBrainzXmlParser : public ThreadWeaver::Job
             ReleaseGroup
         };
 
-        explicit MusicBrainzXmlParser( QString &doc );
+        explicit MusicBrainzXmlParser( const QString &doc );
 
         void run();
 
-- 
cgit v0.11.2