File Fix-crash-during-musicbrainz-search.patch of Package amarok.7290
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
