Separate locks for the different lists managed by XBPython so that they don't interfere with each other. Closes #14048

4 files changed, 103 insertions, 196 deletions

diff --git a/xbmc/interfaces/python/XBPyThread.cpp b/xbmc/interfaces/python/XBPyThread.cpp
index 1b4fb81503..63afa0a9f1 100644
--- a/xbmc/interfaces/python/XBPyThread.cpp
+++ b/xbmc/interfaces/python/XBPyThread.cpp
@@ -261,7 +261,7 @@ void XBPyThread::Process()
 
   // we need to check if we was asked to abort before we had inited
   bool stopping = false;
-  { CSingleLock lock(m_pExecuter->m_critSection);
+  { CSingleLock lock(m_critSec);
     m_threadState = state;
     stopping = m_stopping;
   }
@@ -389,13 +389,13 @@ void XBPyThread::Process()
   PyEval_ReleaseLock();
 
   //set stopped event - this allows ::stop to run and kill remaining threads
-  //this event has to be fired without holding m_pExecuter->m_critSection
-  //before
+  //this event has to be fired without holding m_critSec
+  //
   //Also the GIL (PyEval_AcquireLock) must not be held
   //if not obeyed there is still no deadlock because ::stop waits with timeout (smart one!)
   stoppedEvent.Set();
 
-  { CSingleLock lock(m_pExecuter->m_critSection);
+  { CSingleLock lock(m_critSec);
     m_threadState = NULL;
   }
 
@@ -440,7 +440,7 @@ void XBPyThread::OnException()
   PyThreadState_Swap(NULL);
   PyEval_ReleaseLock();
 
-  CSingleLock lock(m_pExecuter->m_critSection);
+  CSingleLock lock(m_critSec);
   m_threadState = NULL;
   CLog::Log(LOGERROR,"%s, abnormally terminating python thread", __FUNCTION__);
   m_pExecuter->setDone(m_id);
@@ -452,7 +452,7 @@ bool XBPyThread::isStopping() {
 
 void XBPyThread::stop()
 {
-  CSingleLock lock(m_pExecuter->m_critSection);
+  CSingleLock lock(m_critSec);
   if(m_stopping)
     return;
 
@@ -499,12 +499,12 @@ void XBPyThread::stop()
     
     //everything which didn't exit by now gets killed
     {
-      // grabbing the PyLock while holding the XBPython m_critSection is asking for a deadlock
-      CSingleExit ex2(m_pExecuter->m_critSection);
+      // grabbing the PyLock while holding the m_critSec is asking for a deadlock
+      CSingleExit ex2(m_critSec);
       PyEval_AcquireLock();
     }
 
-    // since we released the XBPython m_critSection it's possible that the state is cleaned up 
+    // Since we released the m_critSec it's possible that the state is cleaned up 
     // so we need to recheck for m_threadState == NULL
     if (m_threadState)
     {
diff --git a/xbmc/interfaces/python/XBPyThread.h b/xbmc/interfaces/python/XBPyThread.h
index c49fd959c9..7224120aab 100644
--- a/xbmc/interfaces/python/XBPyThread.h
+++ b/xbmc/interfaces/python/XBPyThread.h
@@ -24,6 +24,7 @@
 
 #include "threads/Thread.h"
 #include "threads/Event.h"
+#include "threads/CriticalSection.h"
 #include "addons/IAddon.h"
 
 class XBPython;
@@ -42,6 +43,7 @@ public:
   void setAddon(ADDON::AddonPtr _addon) { addon = _addon; }
 
 protected:
+  CCriticalSection m_critSec;
   XBPython *m_pExecuter;
   CEvent stoppedEvent;
   void *m_threadState;
diff --git a/xbmc/interfaces/python/XBPython.cpp b/xbmc/interfaces/python/XBPython.cpp
index c8a718e784..ac3d52f942 100644
--- a/xbmc/interfaces/python/XBPython.cpp
+++ b/xbmc/interfaces/python/XBPython.cpp
@@ -79,20 +79,21 @@ XBPython::~XBPython()
   CAnnouncementManager::RemoveAnnouncer(this);
 }
 
+#define LOCK_AND_COPY(type, dest, src) \
+  if (!m_bInitialized) return; \
+  CSingleLock lock(src); \
+  type dest; \
+  dest = src
+
+
 // message all registered callbacks that xbmc stopped playing
 void XBPython::OnPlayBackEnded()
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnPlayBackEnded();
-      it++;
-    }
-  }
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnPlayBackEnded();
+
 }
 
 void XBPython::Announce(AnnouncementFlag flag, const char *sender, const char *message, const CVariant &data)
@@ -124,140 +125,85 @@ void XBPython::Announce(AnnouncementFlag flag, const char *sender, const char *m
 void XBPython::OnPlayBackStarted()
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnPlayBackStarted();
-      it++;
-    }
-  }
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnPlayBackStarted();
 }
 
 // message all registered callbacks that we paused playing
 void XBPython::OnPlayBackPaused()
 {
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnPlayBackPaused();
-      it++;
-    }
-  }
+  TRACE;
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnPlayBackPaused();
 }
 
 // message all registered callbacks that we resumed playing
 void XBPython::OnPlayBackResumed()
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnPlayBackResumed();
-      it++;
-    }
-  }
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnPlayBackResumed();
 }
 
 // message all registered callbacks that user stopped playing
 void XBPython::OnPlayBackStopped()
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnPlayBackStopped();
-      it++;
-    }
-  }
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnPlayBackStopped();
 }
 
 // message all registered callbacks that playback speed changed (FF/RW)
 void XBPython::OnPlayBackSpeedChanged(int iSpeed)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnPlayBackSpeedChanged(iSpeed);
-      it++;
-    }
-  }
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnPlayBackSpeedChanged(iSpeed);
 }
 
 // message all registered callbacks that player is seeking
 void XBPython::OnPlayBackSeek(int iTime, int seekOffset)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnPlayBackSeek(iTime, seekOffset);
-      it++;
-    }
-  }
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnPlayBackSeek(iTime, seekOffset);
 }
 
 // message all registered callbacks that player chapter seeked
 void XBPython::OnPlayBackSeekChapter(int iChapter)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnPlayBackSeekChapter(iChapter);
-      it++;
-    }
-  }
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnPlayBackSeekChapter(iChapter);
 }
 
 // message all registered callbacks that next item has been queued
 void XBPython::OnQueueNextItem()
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
-    while (it != m_vecPlayerCallbackList.end())
-    {
-      ((IPlayerCallback*)(*it))->OnQueueNextItem();
-      it++;
-    }
-  }
+  LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+  for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    ((IPlayerCallback*)(*it))->OnQueueNextItem();
 }
 
 void XBPython::RegisterPythonPlayerCallBack(IPlayerCallback* pCallback)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPlayerCallbackList);
   m_vecPlayerCallbackList.push_back(pCallback);
 }
 
 void XBPython::UnregisterPythonPlayerCallBack(IPlayerCallback* pCallback)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPlayerCallbackList);
   PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
   while (it != m_vecPlayerCallbackList.end())
   {
@@ -271,14 +217,14 @@ void XBPython::UnregisterPythonPlayerCallBack(IPlayerCallback* pCallback)
 void XBPython::RegisterPythonMonitorCallBack(XBMCAddon::xbmc::Monitor* pCallback)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecMonitorCallbackList);
   m_vecMonitorCallbackList.push_back(pCallback);
 }
 
 void XBPython::UnregisterPythonMonitorCallBack(XBMCAddon::xbmc::Monitor* pCallback)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecMonitorCallbackList);
   MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
   while (it != m_vecMonitorCallbackList.end())
   {
@@ -292,99 +238,54 @@ void XBPython::UnregisterPythonMonitorCallBack(XBMCAddon::xbmc::Monitor* pCallba
 void XBPython::OnSettingsChanged(const CStdString &ID)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
-    while (it != m_vecMonitorCallbackList.end())
-    { 
-      if (((XBMCAddon::xbmc::Monitor*)(*it))->GetId() == ID)  
-        ((XBMCAddon::xbmc::Monitor*)(*it))->OnSettingsChanged();
-      it++;
-    }
-  }  
+  LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+  for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    if ((*it)->GetId() == ID)  
+      (*it)->OnSettingsChanged();
 }  
 
 void XBPython::OnScreensaverActivated()
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
-    while (it != m_vecMonitorCallbackList.end())
-    {
-      ((XBMCAddon::xbmc::Monitor*)(*it))->OnScreensaverActivated();
-      it++;
-    }
-  }  
+  LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+  for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    (*it)->OnScreensaverActivated();
 } 
 
 void XBPython::OnScreensaverDeactivated()
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
-    while (it != m_vecMonitorCallbackList.end())
-    {
-      ((XBMCAddon::xbmc::Monitor*)(*it))->OnScreensaverDeactivated();
-      it++;
-    }
-  }  
+  LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+  for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    (*it)->OnScreensaverDeactivated();
 } 
 
 void XBPython::OnDatabaseUpdated(const std::string &database)
 {
   TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
-  MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
-  while (it != m_vecMonitorCallbackList.end())
-  {
-   ((XBMCAddon::xbmc::Monitor*)(*it))->OnDatabaseUpdated(database);
-   it++;
-  }
- }  
+  LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+  for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    (*it)->OnDatabaseUpdated(database);
 } 
 
 void XBPython::OnDatabaseScanStarted(const std::string &database)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
-    while (it != m_vecMonitorCallbackList.end())
-    {
-      ((XBMCAddon::xbmc::Monitor*)(*it))->OnDatabaseScanStarted(database);
-      it++;
-    }
-  }  
+  LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+  for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+    (*it)->OnDatabaseScanStarted(database);
 }
 
 void XBPython::OnAbortRequested(const CStdString &ID)
 {
   TRACE;
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
+  LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+  for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
   {
-    MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
-    while (it != m_vecMonitorCallbackList.end())
-    {
-      if (ID.IsEmpty())
-      {    
-        ((XBMCAddon::xbmc::Monitor*)(*it))->OnAbortRequested();
-      }
-      else
-      {
-        if (((XBMCAddon::xbmc::Monitor*)(*it))->GetId() == ID)
-          ((XBMCAddon::xbmc::Monitor*)(*it))->OnAbortRequested();
-      }
-      it++;
-    }
+    if (ID.IsEmpty())
+      (*it)->OnAbortRequested();
+    else if ((*it)->GetId() == ID)
+      (*it)->OnAbortRequested();
   }  
 } 
 
@@ -633,6 +534,8 @@ void XBPython::FinalizeScript()
   m_endtime = XbmcThreads::SystemClockMillis();
 }
 
+
+// Always called with the lock held on m_critSection
 void XBPython::Finalize()
 {
   TRACE;
@@ -673,19 +576,13 @@ void XBPython::Finalize()
 
 void XBPython::FreeResources()
 {
-  CSingleLock lock(m_critSection);
-  if (m_bInitialized)
-  {
-    // with the m_critSection held, we should copy the PyList so that 
-    // we can operate on the values once we release it.
-    PyList tmpvec = m_vecPyList;
-    m_vecPyList.clear();
+  LOCK_AND_COPY(std::vector<PyElem>,tmpvec,m_vecPyList);
+  m_vecPyList.clear();
 
-    lock.Leave(); //unlock here because the python thread might lock when it exits
+  lock.Leave(); //unlock here because the python thread might lock when it exits
 
-    // cleanup threads that are still running
-    tmpvec.clear(); // boost releases the XBPyThreads which, if deleted, calls FinalizeScript
-  }
+  // cleanup threads that are still running
+  tmpvec.clear(); // boost releases the XBPyThreads which, if deleted, calls FinalizeScript
 }
 
 void XBPython::Process()
@@ -703,7 +600,7 @@ void XBPython::Process()
       CLog::Log(LOGDEBUG, "%s - no profile autoexec.py (%s) found, skipping", __FUNCTION__, strAutoExecPy.c_str());
   }
 
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
 
   if (m_bInitialized)
   {
@@ -723,8 +620,11 @@ void XBPython::Process()
     //delete scripts which are done
     tmpvec.clear(); // boost releases the XBPyThreads which, if deleted, calls FinalizeScript
 
+    CSingleLock l2(m_critSection);
     if(m_iDllScriptCounter == 0 && (XbmcThreads::SystemClockMillis() - m_endtime) > 10000 )
+    {
       Finalize();
+    }
   }
 }
 
@@ -766,7 +666,7 @@ int XBPython::evalFile(const CStdString &src, const std::vector<CStdString> &arg
   if (g_settings.GetCurrentProfile().programsLocked() && !g_passwordManager.IsMasterLockUnlocked(true))
     return -1;
 
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
   Initialize();
 
   if (!m_bInitialized) return -1;
@@ -789,7 +689,7 @@ int XBPython::evalFile(const CStdString &src, const std::vector<CStdString> &arg
 
 void XBPython::setDone(int id)
 {
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
   PyList::iterator it = m_vecPyList.begin();
   while (it != m_vecPyList.end())
   {
@@ -808,7 +708,7 @@ void XBPython::setDone(int id)
 void XBPython::stopScript(int id)
 {
   CSingleExit ex(g_graphicsContext);
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
   PyList::iterator it = m_vecPyList.begin();
   while (it != m_vecPyList.end())
   {
@@ -829,7 +729,7 @@ void* XBPython::getMainThreadState()
 
 int XBPython::ScriptsSize()
 {
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
   return m_vecPyList.size();
 }
 
@@ -837,7 +737,7 @@ const char* XBPython::getFileName(int scriptId)
 {
   const char* cFileName = NULL;
 
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
   PyList::iterator it = m_vecPyList.begin();
   while (it != m_vecPyList.end())
   {
@@ -853,7 +753,7 @@ int XBPython::getScriptId(const CStdString &strFile)
 {
   int iId = -1;
 
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
 
   PyList::iterator it = m_vecPyList.begin();
   while (it != m_vecPyList.end())
@@ -868,7 +768,7 @@ int XBPython::getScriptId(const CStdString &strFile)
 
 bool XBPython::isRunning(int scriptId)
 {
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
 
   for(PyList::iterator it = m_vecPyList.begin(); it != m_vecPyList.end(); it++)
   {
@@ -887,7 +787,7 @@ bool XBPython::isStopping(int scriptId)
 {
   bool bStopping = false;
 
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
   PyList::iterator it = m_vecPyList.begin();
   while (it != m_vecPyList.end())
   {
@@ -901,7 +801,7 @@ bool XBPython::isStopping(int scriptId)
 
 int XBPython::GetPythonScriptId(int scriptPosition)
 {
-  CSingleLock lock(m_critSection);
+  CSingleLock lock(m_vecPyList);
   return (int)m_vecPyList[scriptPosition].id;
 }
 
@@ -946,6 +846,9 @@ int XBPython::evalString(const CStdString &src, const std::vector<CStdString> &a
   inf.strFile   = "<string>";
   inf.pyThread  = pyThread;
 
+  lock.Leave();
+  CSingleLock l2(m_vecPyList);
+
   m_vecPyList.push_back(inf);
 
   return m_nextid;
diff --git a/xbmc/interfaces/python/XBPython.h b/xbmc/interfaces/python/XBPython.h
index a914643f08..2019508bdd 100644
--- a/xbmc/interfaces/python/XBPython.h
+++ b/xbmc/interfaces/python/XBPython.h
@@ -47,10 +47,12 @@ namespace XBMCAddon
   }
 }
 
-typedef std::vector<PyElem> PyList;
-typedef std::vector<PVOID> PlayerCallbackList;
-typedef std::vector<XBMCAddon::xbmc::Monitor*> MonitorCallbackList;
-typedef std::vector<LibraryLoader*> PythonExtensionLibraries;
+template <class T> class LockableType : public T, public CCriticalSection { };
+
+typedef LockableType<std::vector<PVOID> > PlayerCallbackList;
+typedef LockableType<std::vector<XBMCAddon::xbmc::Monitor*> > MonitorCallbackList;
+typedef LockableType<std::vector<PyElem> > PyList;
+typedef LockableType<std::vector<LibraryLoader*> > PythonExtensionLibraries;
 
 class XBPython : 
   public IPlayerCallback,
@@ -128,8 +130,8 @@ public:
   void* getMainThreadState();
 
   bool m_bLogin;
-  CCriticalSection    m_critSection;
 private:
+  CCriticalSection    m_critSection;
   bool              FileExist(const char* strFile);
 
   int               m_nextid;