Commit aa913a59 authored by Matěj Laitl's avatar Matěj Laitl
Browse files

IpodCollection: finally safe solution for crash on early eject

We now (asynchronously) wait for the job that parses iPod tracks and
playlists before destroying IpodCollection.

Other change is that we factor playlists parsing into
IpodParseTracksJob, it really belongs there. (previously it was just
called from that thread and it made false impression that the method
is only called from the main thread)

BUG: 301166
FIXED-IN: 2.6
parent 46baeee2
......@@ -7,6 +7,9 @@ VERSION 2.6
* Building the spectrum analyzer applet can now be disabled with a CMake
argument.
BUGFIXES:
* Don't crash even if the iPod is connected and quickly ejected. (BR 301166)
VERSION 2.6-Beta 1
FEATURES:
* Amazon store: improved album search
......
......@@ -129,6 +129,7 @@ bool IpodCollection::init()
{
// parse tracks in a thread in order not to block main thread
IpodParseTracksJob *job = new IpodParseTracksJob( this );
m_parseTracksJob = job;
connect( job, SIGNAL(done(ThreadWeaver::Job*)), job, SLOT(deleteLater()) );
ThreadWeaver::Weaver::instance()->enqueue( job );
}
......@@ -141,11 +142,7 @@ bool IpodCollection::init()
IpodCollection::~IpodCollection()
{
DEBUG_BLOCK
The::playlistManager()->removeProvider( m_playlistProvider );
// if IpodPlaylistProvider is working in a thread, give it a chance to catch
// IpodCollection destruction.
m_playlistProvider->m_coll = 0;
// this is not racy: destructor should be called in a main thread, the timer fires in the
// same thread
......@@ -342,29 +339,29 @@ IpodCollection::trackForUidUrl( const QString &uidUrl )
void
IpodCollection::slotDestroy()
{
// this is not racy: destroy() is delivered to main thread, the timer fires in the
// this is not racy: slotDestroy() is delivered to main thread, the timer fires in the
// same thread
if( m_writeDatabaseTimer.isActive() )
{
// write database in a thread so that it need not be written in destructor
m_writeDatabaseTimer.stop();
IpodWriteDatabaseJob *job = new IpodWriteDatabaseJob( this );
connect( job, SIGNAL(done(ThreadWeaver::Job*)), SIGNAL(remove()) );
connect( job, SIGNAL(done(ThreadWeaver::Job*)), SLOT(slotRemove()) );
connect( job, SIGNAL(done(ThreadWeaver::Job*)), job, SLOT(deleteLater()) );
ThreadWeaver::Weaver::instance()->enqueue( job );
}
else
emit remove(); // CollectionManager will call deleteLater()
slotRemove();
}
void
IpodCollection::slotEject()
{
// this is not racy: destroy() is delivered to main thread, the timer fires in the
// this is not racy: slotEject() is delivered to main thread, the timer fires in the
// same thread
if( m_writeDatabaseTimer.isActive() )
{
// write database now because iPod will be unmounted in destructor
// write database now because iPod will be already unmounted in destructor
m_writeDatabaseTimer.stop();
IpodWriteDatabaseJob *job = new IpodWriteDatabaseJob( this );
connect( job, SIGNAL(done(ThreadWeaver::Job*)), SLOT(slotPerformTeardownAndRemove()) );
......@@ -524,7 +521,23 @@ void IpodCollection::slotPerformTeardownAndRemove()
ssa->teardown();
}
emit remove(); // CollectionManager will call deleteLater()
slotRemove();
}
void IpodCollection::slotRemove()
{
// this is not racy, we are in the main thread and parseTracksJob can be deleted only
// in the main thread
if( m_parseTracksJob )
{
// we need to wait until parseTracksJob finishes, because it acceses IpodCollection
// and IpodPlaylistProvider in an asynchronous way that cannot safely cope with
// IpodCollection disappearing
connect( m_parseTracksJob.data(), SIGNAL(destroyed(QObject*)), SIGNAL(remove()) );
m_parseTracksJob.data()->abort();
}
else
emit remove();
}
Meta::TrackPtr
......
......@@ -28,6 +28,7 @@
namespace Collections { class MemoryCollection; }
namespace IpodMeta { class Track; }
class IphoneMountPoint;
class IpodParseTracksJob;
class IpodPlaylistProvider;
class QTemporaryFile;
struct _Itdb_iTunesDB;
......@@ -200,6 +201,12 @@ class IpodCollection : public Collections::Collection, public Meta::Observer
*/
void slotPerformTeardownAndRemove();
/**
* Do sanity checks and emit remove() so that this collection is destroyed by
* CollectionManager. No other method is allowed to emit remove()!
*/
void slotRemove();
private:
friend class IpodCopyTracksJob;
friend class IpodDeleteTracksJob;
......@@ -267,6 +274,7 @@ class IpodCollection : public Collections::Collection, public Meta::Observer
QAction *m_configureAction;
QAction *m_ejectAction;
QAction *m_consolidateAction;
QWeakPointer<IpodParseTracksJob> m_parseTracksJob;
};
#endif // IPODCOLLECTION_H
......@@ -32,9 +32,7 @@ IpodPlaylist::IpodPlaylist( Itdb_Playlist *ipodPlaylist, IpodCollection *collect
, m_coll( collection )
, m_type( Normal )
{
Q_ASSERT( m_playlist );
if( !m_coll )
return; // corner case, can happen if IpodCollection is destructed in wrong time
Q_ASSERT( m_playlist && collection );
for( GList *members = m_playlist->members; members; members = members->next )
{
Itdb_Track *itdbTrack = (Itdb_Track *) members->data;
......
......@@ -24,9 +24,7 @@
#include "core/support/Components.h"
#include "core/support/Debug.h"
#include "core-impl/collections/support/FileCollectionLocation.h"
#include "core-impl/meta/file/File.h"
#include <glib.h>
#include <gpod/itdb.h>
......@@ -217,52 +215,6 @@ IpodPlaylistProvider::removeTrackFromPlaylists( Meta::TrackPtr track )
}
}
void
IpodPlaylistProvider::parseItdbPlaylists( const Meta::TrackList &staleTracks, const QSet<QString> &knownPaths )
{
if( !staleTracks.isEmpty() )
{
m_stalePlaylist = Playlists::PlaylistPtr( new IpodPlaylist( staleTracks,
i18nc( "iPod playlist name", "Stale tracks" ), m_coll, IpodPlaylist::Stale ) );
m_playlists << m_stalePlaylist; // we dont subscribe to this playlist, no need to update database
emit playlistAdded( m_stalePlaylist );
}
Meta::TrackList orphanedTracks = findOrphanedTracks( knownPaths );
if( !orphanedTracks.isEmpty() )
{
m_orphanedPlaylist = Playlists::PlaylistPtr( new IpodPlaylist( orphanedTracks,
i18nc( "iPod playlist name", "Orphaned tracks" ), m_coll, IpodPlaylist::Orphaned ) );
m_playlists << m_orphanedPlaylist; // we dont subscribe to this playlist, no need to update database
emit playlistAdded( m_orphanedPlaylist );
}
Itdb_iTunesDB *itdb = m_coll ? m_coll->m_itdb : 0;
for( GList *playlists = itdb ? itdb->playlists : 0; playlists; playlists = playlists->next )
{
if( !m_coll )
return; // guard against IpodCollection being destructed behind our back
Itdb_Playlist *playlist = (Itdb_Playlist *) playlists->data;
if( !playlist )
continue;
if( itdb_playlist_is_mpl( playlist ) )
continue; // skip master playlist
Playlists::PlaylistPtr playlistPtr( new IpodPlaylist( playlist, m_coll ) );
m_playlists << playlistPtr;
subscribeTo( playlistPtr );
emit playlistAdded( playlistPtr );
}
if( m_stalePlaylist || m_orphanedPlaylist )
{
QString text = i18n( "Stale and/or orphaned tracks detected on %1. You can resolve "
"the situation using the <b>%2</b> collection action. You can also view the tracks "
"under the Saved Playlists tab.", m_coll->prettyName(),
m_coll->m_consolidateAction->text() );
Amarok::Components::logger()->longMessage( text );
}
}
bool
IpodPlaylistProvider::hasStaleOrOrphaned() const
{
......@@ -428,37 +380,6 @@ void IpodPlaylistProvider::copyAndInsertToPlaylist( const TrackPositionList &tra
}
}
Meta::TrackList
IpodPlaylistProvider::findOrphanedTracks( const QSet<QString> &knownPaths )
{
gchar *musicDirChar = itdb_get_music_dir( QFile::encodeName( m_coll->mountPoint() ) );
QString musicDirPath = QFile::decodeName( musicDirChar );
g_free( musicDirChar );
musicDirChar = 0;
QStringList trackPatterns;
foreach( QString suffix, m_coll->supportedFormats() )
{
trackPatterns << QString( "*.%1" ).arg( suffix );
}
Meta::TrackList orphanedTracks;
QDir musicDir( musicDirPath );
foreach( QString subdir, musicDir.entryList( QStringList( "F??" ), QDir::Dirs | QDir::NoDotAndDotDot ) )
{
subdir = musicDir.absoluteFilePath( subdir ); // make the path absolute
foreach( QFileInfo info, QDir( subdir ).entryInfoList( trackPatterns ) )
{
QString canonPath = info.canonicalFilePath();
if( knownPaths.contains( canonPath ) )
continue; // already in iTunes database
Meta::TrackPtr track( new MetaFile::Track( KUrl( canonPath ) ) );
orphanedTracks << track;
}
}
return orphanedTracks;
}
bool
IpodPlaylistProvider::orphanedAndStaleTracksMatch( const Meta::TrackPtr &orphaned, const Meta::TrackPtr &stale )
{
......
......@@ -75,17 +75,6 @@ class IpodPlaylistProvider : public Playlists::UserPlaylistProvider, private Pla
*/
void removeTrackFromPlaylists( Meta::TrackPtr track );
/**
* Go through iPod playlists and create Amarok playlists for them. Designed to be
* run from non-gui thread from IpodParseTracksJob.
*
* @param staleTracks list of track from iTunes database whose associated file
* no longer exists
* @param knownPaths a set of absolute local paths of all track from iTunes
* database; used for orphaned tracks detection
*/
void parseItdbPlaylists( const Meta::TrackList &staleTracks, const QSet<QString> &knownPaths );
/**
* Return true whether there are some stale & orphaned files/entries in iTunes db
*/
......@@ -109,10 +98,9 @@ class IpodPlaylistProvider : public Playlists::UserPlaylistProvider, private Pla
void slotCopyAndInsertToPlaylists();
private:
friend class IpodCollection;
friend class IpodParseTracksJob;
void copyAndInsertToPlaylist( const TrackPositionList &tracks, Playlists::PlaylistPtr destPlaylist );
Meta::TrackList findOrphanedTracks( const QSet<QString> &knownPaths );
bool orphanedAndStaleTracksMatch( const Meta::TrackPtr &orphaned, const Meta::TrackPtr &stale );
template <class T> bool entitiesDiffer( T first, T second );
......
......@@ -16,10 +16,13 @@
#include "IpodParseTracksJob.h"
#include "IpodCollection.h"
#include "IpodMeta.h"
#include "IpodPlaylistProvider.h"
#include "core/interfaces/Logger.h"
#include "core/support/Components.h"
#include "core/support/Debug.h"
#include "core-impl/meta/file/File.h"
#include <gpod/itdb.h>
......@@ -31,13 +34,19 @@ IpodParseTracksJob::IpodParseTracksJob( IpodCollection *collection )
{
}
void IpodParseTracksJob::abort()
{
m_aborted = true;
}
void
IpodParseTracksJob::run()
{
if( !m_coll || !m_coll.data()->m_itdb )
return;
DEBUG_BLOCK
Itdb_iTunesDB *itdb = m_coll->m_itdb;
if( !itdb )
return; // paranoia
Itdb_iTunesDB *itdb = m_coll.data()->m_itdb;
guint32 trackNumber = itdb_tracks_number( itdb );
QString operationText = i18nc( "operation when iPod is connected", "Reading iPod tracks" );
Amarok::Components::logger()->newProgressOperation( this, operationText, trackNumber,
......@@ -48,14 +57,14 @@ IpodParseTracksJob::run()
for( GList *tracklist = itdb->tracks; tracklist; tracklist = tracklist->next )
{
if( m_aborted || !m_coll )
if( m_aborted )
break;
Itdb_Track *ipodTrack = (Itdb_Track *) tracklist->data;
if( !ipodTrack )
continue; // paranoia
// IpodCollection::addTrack() locks and unlocks the m_itdbMutex mutex
Meta::TrackPtr proxyTrack = m_coll.data()->addTrack( new IpodMeta::Track( ipodTrack ) );
Meta::TrackPtr proxyTrack = m_coll->addTrack( new IpodMeta::Track( ipodTrack ) );
if( proxyTrack )
{
QString canonPath = QFileInfo( proxyTrack->playableUrl().toLocalFile() ).canonicalFilePath();
......@@ -68,16 +77,86 @@ IpodParseTracksJob::run()
incrementProgress();
}
IpodPlaylistProvider *provider = m_coll ? m_coll.data()->m_playlistProvider : 0;
if( provider )
provider->parseItdbPlaylists( staleTracks, knownPaths );
parsePlaylists( staleTracks, knownPaths );
emit endProgressOperation( this );
}
void IpodParseTracksJob::abort()
void
IpodParseTracksJob::parsePlaylists( const Meta::TrackList &staleTracks,
const QSet<QString> &knownPaths )
{
m_aborted = true;
IpodPlaylistProvider *prov = m_coll->m_playlistProvider;
if( !prov || m_aborted )
return;
if( !staleTracks.isEmpty() )
{
prov->m_stalePlaylist = Playlists::PlaylistPtr( new IpodPlaylist( staleTracks,
i18nc( "iPod playlist name", "Stale tracks" ), m_coll, IpodPlaylist::Stale ) );
prov->m_playlists << prov->m_stalePlaylist; // we dont subscribe to this playlist, no need to update database
emit prov->playlistAdded( prov->m_stalePlaylist );
}
Meta::TrackList orphanedTracks = findOrphanedTracks( knownPaths );
if( !orphanedTracks.isEmpty() )
{
prov->m_orphanedPlaylist = Playlists::PlaylistPtr( new IpodPlaylist( orphanedTracks,
i18nc( "iPod playlist name", "Orphaned tracks" ), m_coll, IpodPlaylist::Orphaned ) );
prov->m_playlists << prov->m_orphanedPlaylist; // we dont subscribe to this playlist, no need to update database
emit prov->playlistAdded( prov->m_orphanedPlaylist );
}
if( !m_coll->m_itdb || m_aborted )
return;
for( GList *playlists = m_coll->m_itdb->playlists; playlists; playlists = playlists->next )
{
Itdb_Playlist *playlist = (Itdb_Playlist *) playlists->data;
if( !playlist || itdb_playlist_is_mpl( playlist ) )
continue; // skip null (?) or master playlists
Playlists::PlaylistPtr playlistPtr( new IpodPlaylist( playlist, m_coll ) );
prov->m_playlists << playlistPtr;
prov->subscribeTo( playlistPtr );
emit prov->playlistAdded( playlistPtr );
}
if( !m_aborted && ( prov->m_stalePlaylist || prov->m_orphanedPlaylist ) )
{
QString text = i18n( "Stale and/or orphaned tracks detected on %1. You can resolve "
"the situation using the <b>%2</b> collection action. You can also view the tracks "
"under the Saved Playlists tab.", m_coll->prettyName(),
m_coll->m_consolidateAction->text() );
Amarok::Components::logger()->longMessage( text );
}
}
#include "IpodParseTracksJob.moc"
Meta::TrackList IpodParseTracksJob::findOrphanedTracks(const QSet< QString >& knownPaths)
{
gchar *musicDirChar = itdb_get_music_dir( QFile::encodeName( m_coll->mountPoint() ) );
QString musicDirPath = QFile::decodeName( musicDirChar );
g_free( musicDirChar );
musicDirChar = 0;
QStringList trackPatterns;
foreach( QString suffix, m_coll->supportedFormats() )
{
trackPatterns << QString( "*.%1" ).arg( suffix );
}
Meta::TrackList orphanedTracks;
QDir musicDir( musicDirPath );
foreach( QString subdir, musicDir.entryList( QStringList( "F??" ), QDir::Dirs | QDir::NoDotAndDotDot ) )
{
if( m_aborted )
return Meta::TrackList();
subdir = musicDir.absoluteFilePath( subdir ); // make the path absolute
foreach( QFileInfo info, QDir( subdir ).entryInfoList( trackPatterns ) )
{
QString canonPath = info.canonicalFilePath();
if( knownPaths.contains( canonPath ) )
continue; // already in iTunes database
Meta::TrackPtr track( new MetaFile::Track( KUrl( canonPath ) ) );
orphanedTracks << track;
}
}
return orphanedTracks;
}
......@@ -17,24 +17,33 @@
#ifndef IPODPARSETRACKSJOB_H
#define IPODPARSETRACKSJOB_H
#include "IpodCollection.h"
#include "core/meta/Meta.h"
#include <ThreadWeaver/Job>
#include <QWeakPointer>
class IpodCollection;
/**
* A job designed to parse iPod tracks and playlists in a thread so that main thread is
* not blocked with it. It is guaranteed by IpodCollection that is doesn't destory itself
* while this job is alive. Memory management of this job is up to the caller of it.
*/
class IpodParseTracksJob : public ThreadWeaver::Job
{
Q_OBJECT
public:
explicit IpodParseTracksJob( IpodCollection *collection );
virtual void run();
public slots:
/**
* Aborts the job as soon as it is safely possible
*/
void abort();
protected:
void run();
signals:
// signals for progress operation:
void incrementProgress();
......@@ -42,7 +51,19 @@ class IpodParseTracksJob : public ThreadWeaver::Job
void totalSteps( int steps ); // not used, defined to keep QObject::conect warning quiet
private:
QWeakPointer<IpodCollection> m_coll;
/**
* Go through iPod playlists and create Amarok playlists for them.
*
* @param staleTracks list of track from iTunes database whose associated file
* no longer exists
* @param knownPaths a set of absolute local paths of all track from iTunes
* database; used for orphaned tracks detection
*/
void parsePlaylists( const Meta::TrackList &staleTracks,
const QSet<QString> &knownPaths );
Meta::TrackList findOrphanedTracks( const QSet<QString> &knownPaths );
IpodCollection *m_coll;
bool m_aborted;
};
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment