Avoid races when using OSX_RUNINMAIN_MEMBERS

...so that e.g. main thread in SalYieldMutex::doAcquire could reset
m_aInMainTHreadCondition and then block waiting on it only after another thread
had set it.  (Saw such a deadlock in some 'make check' CppunitTest.)

Change-Id: I5c676956a2bec6bf8f94d7dbeee64f100db39bd3
Reviewed-on: https://gerrit.libreoffice.org/44501Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

vcl/inc/osx/runinmain.hxx

@@ -57,8 +57,11 @@ union RuninmainResult
 };
 
 #define OSX_RUNINMAIN_MEMBERS \
-    osl::Condition          m_aInMainCondition; \
-    osl::Condition          m_aResultCondition; \
+    std::mutex              m_runInMainMutex; \
+    std::condition_variable m_aInMainCondition; \
+    std::condition_variable m_aResultCondition; \
+    bool                    m_wakeUpMain = false; \
+    bool                    m_resultReady = false; \
     RuninmainBlock          m_aCodeBlock; \
     RuninmainResult         m_aResult;
 
@@ -67,18 +70,24 @@ union RuninmainResult
     { \
         DBG_TESTSOLARMUTEX(); \
         SalYieldMutex *aMutex = static_cast<SalYieldMutex*>(instance->GetYieldMutex()); \
-        assert( !aMutex->m_aCodeBlock ); \
-        aMutex->m_aCodeBlock = Block_copy(^{ \
-            command; \
-        }); \
-        aMutex->m_aResultCondition.reset(); \
+        { \
+            std::unique_lock<std::mutex> g(aMutex->m_runInMainMutex); \
+            assert( !aMutex->m_aCodeBlock ); \
+            aMutex->m_aCodeBlock = Block_copy(^{ \
+                command; \
+            }); \
+            aMutex->m_wakeUpMain = true; \
+            aMutex->m_aInMainCondition.notify_all(); \
+        } \
         dispatch_async(dispatch_get_main_queue(),^{ \
             ImplNSAppPostEvent( AquaSalInstance::YieldWakeupEvent, NO ); \
             }); \
-        aMutex->m_aInMainCondition.set(); \
-        osl::Condition::Result res =  aMutex->m_aResultCondition.wait(); \
-        (void) res; \
-        assert(osl::Condition::Result::result_ok == res); \
+        { \
+            std::unique_lock<std::mutex> g(aMutex->m_runInMainMutex); \
+            aMutex->m_aResultCondition.wait( \
+                g, [&aMutex]() { return aMutex->m_resultReady; }); \
+            aMutex->m_resultReady = false; \
+        } \
         return; \
     }
 
@@ -87,18 +96,24 @@ union RuninmainResult
     { \
         DBG_TESTSOLARMUTEX(); \
         SalYieldMutex *aMutex = static_cast<SalYieldMutex*>(instance->GetYieldMutex()); \
-        assert( !aMutex->m_aCodeBlock ); \
-        aMutex->m_aCodeBlock = Block_copy(^{ \
-            aMutex->m_aResult.pointer = static_cast<void*>( command ); \
-        }); \
-        aMutex->m_aResultCondition.reset(); \
+        { \
+            std::unique_lock<std::mutex> g(aMutex->m_runInMainMutex); \
+            assert( !aMutex->m_aCodeBlock ); \
+            aMutex->m_aCodeBlock = Block_copy(^{ \
+                aMutex->m_aResult.pointer = static_cast<void*>( command ); \
+            }); \
+            aMutex->m_wakeUpMain = true; \
+            aMutex->m_aInMainCondition.notify_all(); \
+        } \
         dispatch_async(dispatch_get_main_queue(),^{ \
             ImplNSAppPostEvent( AquaSalInstance::YieldWakeupEvent, NO ); \
             }); \
-        aMutex->m_aInMainCondition.set(); \
-        osl::Condition::Result res =  aMutex->m_aResultCondition.wait(); \
-        (void) res; \
-        assert(osl::Condition::Result::result_ok == res); \
+        { \
+            std::unique_lock<std::mutex> g(aMutex->m_runInMainMutex); \
+            aMutex->m_aResultCondition.wait( \
+                g, [&aMutex]() { return aMutex->m_resultReady; }); \
+            aMutex->m_resultReady = false; \
+        } \
         return static_cast<type>( aMutex->m_aResult.pointer ); \
     }
 
@@ -107,18 +122,24 @@ union RuninmainResult
     { \
         DBG_TESTSOLARMUTEX(); \
         SalYieldMutex *aMutex = static_cast<SalYieldMutex*>(instance->GetYieldMutex()); \
-        assert( !aMutex->m_aCodeBlock ); \
-        aMutex->m_aCodeBlock = Block_copy(^{ \
-            aMutex->m_aResult.member = command; \
-        }); \
-        aMutex->m_aResultCondition.reset(); \
+        { \
+            std::unique_lock<std::mutex> g(aMutex->m_runInMainMutex); \
+            assert( !aMutex->m_aCodeBlock ); \
+            aMutex->m_aCodeBlock = Block_copy(^{ \
+                aMutex->m_aResult.member = command; \
+            }); \
+            aMutex->m_wakeUpMain = true; \
+            aMutex->m_aInMainCondition.notify_all(); \
+        } \
         dispatch_async(dispatch_get_main_queue(),^{ \
             ImplNSAppPostEvent( AquaSalInstance::YieldWakeupEvent, NO ); \
             }); \
-        aMutex->m_aInMainCondition.set(); \
-        osl::Condition::Result res =  aMutex->m_aResultCondition.wait(); \
-        (void) res; \
-        assert(osl::Condition::Result::result_ok == res); \
+        { \
+            std::unique_lock<std::mutex> g(aMutex->m_runInMainMutex); \
+            aMutex->m_aResultCondition.wait( \
+                g, [&aMutex]() { return aMutex->m_resultReady; }); \
+            aMutex->m_resultReady = false; \
+        } \
         return std::move( aMutex->m_aResult.member ); \
     }
 

vcl/inc/osx/salinst.h

@@ -21,7 +21,11 @@
 #ifndef INCLUDED_VCL_INC_OSX_SALINST_H
 #define INCLUDED_VCL_INC_OSX_SALINST_H
 
+#include <sal/config.h>
+
+#include <condition_variable>
 #include <list>
+#include <mutex>
 
 #include <comphelper/solarmutex.hxx>
 #include <osl/conditn.hxx>

vcl/osx/salinst.cxx

@@ -17,6 +17,12 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include <sal/config.h>
+
+#include <condition_variable>
+#include <mutex>
+#include <utility>
+
 #include <config_features.h>
 
 #include <stdio.h>
@@ -277,24 +283,31 @@ void SalYieldMutex::doAcquire( sal_uInt32 nLockCount )
         if ( pInst->mbNoYieldLock )
             return;
         do {
-            m_aInMainCondition.reset();
-            if ( m_aCodeBlock )
+            RuninmainBlock block = nullptr;
+            {
+                std::unique_lock<std::mutex> g(m_runInMainMutex);
+                if (m_aMutex.tryToAcquire()) {
+                    assert(m_aCodeBlock == nullptr);
+                    m_wakeUpMain = false;
+                    break;
+                }
+                // wait for doRelease() or RUNINMAIN_* to set the condition
+                m_aInMainCondition.wait(g, [this]() { return m_wakeUpMain; });
+                m_wakeUpMain = false;
+                std::swap(block, m_aCodeBlock);
+            }
+            if ( block )
             {
                 assert( !pInst->mbNoYieldLock );
                 pInst->mbNoYieldLock = true;
-                m_aCodeBlock();
+                block();
                 pInst->mbNoYieldLock = false;
-                Block_release( m_aCodeBlock );
-                m_aCodeBlock = nullptr;
-                m_aResultCondition.set();
+                Block_release( block );
+                std::unique_lock<std::mutex> g(m_runInMainMutex);
+                assert(!m_resultReady);
+                m_resultReady = true;
+                m_aResultCondition.notify_all();
             }
-            // reset condition *before* acquiring!
-            if (m_aMutex.tryToAcquire())
-                break;
-            // wait for doRelease() or RUNINMAIN_* to set the condition
-            osl::Condition::Result res =  m_aInMainCondition.wait();
-            (void) res;
-            assert(osl::Condition::Result::result_ok == res);
         }
         while ( true );
     }
@@ -311,11 +324,15 @@ sal_uInt32 SalYieldMutex::doRelease( const bool bUnlockAll )
     AquaSalInstance *pInst = GetSalData()->mpInstance;
     if ( pInst->mbNoYieldLock && pInst->IsMainThread() )
         return 1;
-    sal_uInt32 nCount = comphelper::GenericSolarMutex::doRelease( bUnlockAll );
-
-    if ( 0 == m_nCount && !pInst->IsMainThread() )
-        m_aInMainCondition.set();
-
+    sal_uInt32 nCount;
+    {
+        std::unique_lock<std::mutex> g(m_runInMainMutex);
+        nCount = comphelper::GenericSolarMutex::doRelease( bUnlockAll );
+        if ( 0 == m_nCount && !pInst->IsMainThread() ) {
+            m_wakeUpMain = true;
+            m_aInMainCondition.notify_all();
+        }
+    }
     return nCount;
 }