[amarok] src: Prevent hang in testmetamultitrack.

[Matěj Laitl]

On 11. 7. 2012 Ralf Engels wrote:
> Git commit 115cb80f9bd94b23640ca9245c97d6c8e25d2c97 by Ralf Engels.
> Committed on 11/07/2012 at 16:25.
> Pushed by rengels into branch 'master'.
> 
> Prevent hang in testmetamultitrack.
> 
> The class variable mutex had problems in the destructor.
> The function static mutex should not have the problem and I also
> can't see problems with initialization.

This is wrong. C++98 standard doesn't guarantee that initialization of 
function-static variables happens in a thread-safe way [1] (C++11 does [2], so 
does gcc even in C++98 [3]) Initialization of static _class_ variables doesn't 
suffer from this problem.

This can lead to nasty things like initializing the mutex from 2 threads 
simultaneously which will probably lead to incorrect operation of it, e.g. you 
effectively circumvent the purpose of the mutex.

I introduced the class mutex in commit 8db5599d in order to fix bug 300659. 
This change must be reverted, else it will reintroduce bug 300659 with non-gcc 
compilers or with -fno-threadsafe-statics.

Because I'm the author of the original change, I'll take responsibility to fix 
the test that is broken by it.

[1] http://blogs.msdn.com/b/oldnewthing/archive/2004/03/08/85901.aspx
[2] http://stackoverflow.com/questions/8102125
[3] http://stackoverflow.com/questions/1270927

Regards,
			Matěj

> M  +3    -3    src/EngineController.cpp
> M  +0    -2    src/EngineController.h
> 
> http://commits.kde.org/amarok/115cb80f9bd94b23640ca9245c97d6c8e25d2c97
> 
> diff --git a/src/EngineController.cpp b/src/EngineController.cpp
> index ed55687..736fa12 100644
> --- a/src/EngineController.cpp
> +++ b/src/EngineController.cpp
> @@ -58,8 +58,6 @@ namespace The {
>      EngineController* engineController() { return
> EngineController::instance(); } }
> 
> -QMutex EngineController::s_supportedMimeTypesMutex;
> -
>  EngineController*
>  EngineController::instance()
>  {
> @@ -264,13 +262,15 @@ EngineController::supportedMimeTypes() //static
>  {
>      //NOTE this function must be thread-safe
> 
> +    // mutex to protect the mimetable and the mimeTableAlreadyFilled status
> +    static QMutex supportedMimeTypesMutex;
>      // Filter the available mime types to only include audio and video, as
> amarok does not intend to play photos static QStringList mimeTable;
>      // theoretically not needed, but static initialization of mimeTable may
> have threading // issues, so rather use boolean flag for it:
>      static bool mimeTableAlreadyFilled = false;
> 
> -    QMutexLocker locker( &s_supportedMimeTypesMutex );
> +    QMutexLocker locker( &supportedMimeTypesMutex );
>      if( mimeTableAlreadyFilled )
>          return mimeTable;
> 
> diff --git a/src/EngineController.h b/src/EngineController.h
> index ad2e0c4..3da2f23 100644
> --- a/src/EngineController.h
> +++ b/src/EngineController.h
> @@ -538,8 +538,6 @@ private:
> 
>      Q_DISABLE_COPY( EngineController )
> 
> -    static QMutex s_supportedMimeTypesMutex; // guards access to
> supportedMimeTypes()::mimeTable
> -
>      QWeakPointer<Phonon::MediaObject>       m_media;
>      QWeakPointer<Phonon::VolumeFaderEffect> m_preamp;
>      QWeakPointer<Phonon::Effect>            m_equalizer;