tdf#96887 enhance SolarMutex AcquireWithWait for Windows

Currently the Windows-specific method ImplSalYieldMutexAcquireWithWait()
uses a messaging mechanism to learn about the SolarMutex being free again.
This is not reliable when the MessageQueue overflows (MS allows 10000
messages per queue). It is more safe to use MsgWaitForMultipleObjects.
This also allows to not only wait for the SolarMutex to be freed, but
also to detect when SendMessage() is used which needs to lead to a
reschedule to not block current Window handling.

Change-Id: Id317dda62aaa1fe7677d8d28929e6936e5a22705
Reviewed-on: https://gerrit.libreoffice.org/23921
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Armin Le Grand <Armin.Le.Grand@cib.de>

3 files changed, 34 insertions, 93 deletions

diff --git a/vcl/inc/win/saldata.hxx b/vcl/inc/win/saldata.hxx
index ed8a2fd9a2ec..66b8a7fd7179 100644
--- a/vcl/inc/win/saldata.hxx
+++ b/vcl/inc/win/saldata.hxx
@@ -214,8 +214,6 @@ int ImplSalWICompareAscii( const wchar_t* pStr1, const char* pStr2 );
 
 // wParam == bWait; lParam == 0
 #define SAL_MSG_THREADYIELD         (WM_USER+111)
-// wParam == 0; lParam == 0
-#define SAL_MSG_RELEASEWAITYIELD    (WM_USER+112)
 // wParam == 0; lParam == nMS
 #define SAL_MSG_STARTTIMER          (WM_USER+113)
 // wParam == nFrameStyle; lParam == pParent; lResult == pFrame
diff --git a/vcl/inc/win/salinst.h b/vcl/inc/win/salinst.h
index b6408ea4f322..fe2c3414593e 100644
--- a/vcl/inc/win/salinst.h
+++ b/vcl/inc/win/salinst.h
@@ -33,14 +33,6 @@ public:
     HWND                mhComWnd;
     /// The Yield mutex ensures that only one thread calls into VCL
     SalYieldMutex*      mpSalYieldMutex;
-    /// The Wait mutex ensures increment of mnYieldWaitCount and acquisition
-    /// or release of mpSalYieldMutex is atomic
-    osl::Mutex*         mpSalWaitMutex;
-    /// count main thread's pending ImplSalYieldMutexAcquireWithWait() calls
-    /// (it's not clear to me if this will be > 1 in practice; it would be
-    /// possible if main thread's handling of SAL_MSG_* sent by other threads
-    /// via SendMessage() ends up calling ImplSalYieldMutexAcquireWithWait())
-    sal_uInt16          mnYieldWaitCount;
 
 public:
     WinSalInstance();
diff --git a/vcl/win/app/salinst.cxx b/vcl/win/app/salinst.cxx
index 1e37ece50411..a44e93068f7e 100644
--- a/vcl/win/app/salinst.cxx
+++ b/vcl/win/app/salinst.cxx
@@ -109,9 +109,9 @@ LRESULT CALLBACK SalComWndProcW( HWND hWnd, UINT nMsg, WPARAM wParam, LPARAM lPa
 
 class SalYieldMutex : public comphelper::SolarMutex
 {
-    osl::Mutex m_mutex;
-
-public: // for ImplSalYield()
+public: // for ImplSalYield() and ImplSalYieldMutexAcquireWithWait()
+    osl::Mutex                  m_mutex;
+    osl::Condition              m_condition; /// for MsgWaitForMultipleObjects()
     WinSalInstance*             mpInstData;
     sal_uLong                   mnCount;
     DWORD                       mnThreadId;
@@ -147,10 +147,11 @@ void SalYieldMutex::release()
         m_mutex.release();
     else
     {
-        SalData* pSalData = GetSalData();
-        if ( pSalData->mnAppThreadId != nThreadId )
+        bool isRelease(1 == mnCount);
+        if ( isRelease )
         {
-            if ( mnCount == 1 )
+            SalData* pSalData = GetSalData();
+            if ( pSalData->mnAppThreadId != nThreadId )
             {
                 OpenGLContext::prepareForYield();
 
@@ -158,31 +159,21 @@ void SalYieldMutex::release()
                 // Java clients doesn't come in the right order
                 GdiFlush();
 
-                // lock here to ensure that the test of mnYieldWaitCount and
-                // m_mutex.release() is atomic
-                mpInstData->mpSalWaitMutex->acquire();
-                if ( mpInstData->mnYieldWaitCount )
-                    PostMessageW( mpInstData->mhComWnd, SAL_MSG_RELEASEWAITYIELD, 0, 0 );
                 mnThreadId = 0;
-                mnCount--;
-                m_mutex.release();
-                mpInstData->mpSalWaitMutex->release();
             }
             else
             {
-                mnCount--;
-                m_mutex.release();
-            }
-        }
-        else
-        {
-            if ( mnCount == 1 )
-            {
                 mnThreadId = 0;
                 OpenGLContext::prepareForYield();
             }
-            mnCount--;
-            m_mutex.release();
+        }
+
+        mnCount--;
+        m_mutex.release();
+
+        if ( isRelease )
+        {   // do this *after* release
+            m_condition.set(); // wake up ImplSalYieldMutexAcquireWithWait()
         }
     }
 }
@@ -216,57 +207,33 @@ void ImplSalYieldMutexAcquireWithWait()
     if ( !pInst )
         return;
 
-    // If this is the main thread, then we must wait with GetMessage(),
-    // because if we don't reschedule, then we create deadlocks if a Window's
-    // create/destroy is called via SendMessage() from another thread.
-    // If this is not the main thread, call acquire directly.
     DWORD nThreadId = GetCurrentThreadId();
     SalData* pSalData = GetSalData();
+
     if ( pSalData->mnAppThreadId == nThreadId )
     {
-        // wait till we get the Mutex
-        bool bAcquire = false;
-        do
+        // tdf#96887 If this is the main thread, then we must wait for two things:
+        // - the mpSalYieldMutex being freed
+        // - SendMessage() being triggered
+        // This can nicely be done using MsgWaitForMultipleObjects. The 2nd one is
+        // needed because if we don't reschedule, then we create deadlocks if a
+        // Window's create/destroy is called via SendMessage() from another thread.
+        // Have a look at the osl_waitCondition implementation for more info.
+        pInst->mpSalYieldMutex->m_condition.reset();
+        while (!pInst->mpSalYieldMutex->tryToAcquire())
         {
-            if ( pInst->mpSalYieldMutex->tryToAcquire() )
-                bAcquire = true;
-            else
-            {
-                pInst->mpSalWaitMutex->acquire();
-                if ( pInst->mpSalYieldMutex->tryToAcquire() )
-                {
-                    bAcquire = true;
-                    pInst->mpSalWaitMutex->release();
-                }
-                else
-                {
-                    // other threads must not observe mnYieldWaitCount == 0
-                    // while main thread is blocked in GetMessage()
-                    pInst->mnYieldWaitCount++;
-                    pInst->mpSalWaitMutex->release();
-                    MSG aTmpMsg;
-                    // this call exists because it dispatches SendMessage() msg!
-                    GetMessageW( &aTmpMsg, pInst->mhComWnd, SAL_MSG_RELEASEWAITYIELD, SAL_MSG_RELEASEWAITYIELD );
-                    // it is possible that another thread acquires and releases
-                    // mpSalYieldMutex after the GetMessage call returns,
-                    // observes mnYieldWaitCount != 0 and sends an extra
-                    // SAL_MSG_RELEASEWAITYIELD - but that appears unproblematic
-                    // as it will just cause the next Yield to do an extra
-                    // iteration of the while loop here
-                    pInst->mnYieldWaitCount--;
-                    if ( pInst->mnYieldWaitCount )
-                    {
-                        // repeat the message so that the next instance of this
-                        // function further up the call stack is unblocked too
-                        PostMessageW( pInst->mhComWnd, SAL_MSG_RELEASEWAITYIELD, 0, 0 );
-                    }
-                }
-            }
+            // wait for SalYieldMutex::release() to set the condition
+            osl::Condition::Result res = pInst->mpSalYieldMutex->m_condition.wait();
+            assert(osl::Condition::Result::result_ok == res);
+            // reset condition *before* acquiring!
+            pInst->mpSalYieldMutex->m_condition.reset();
         }
-        while ( !bAcquire );
     }
     else
+    {
+        // If this is not the main thread, call acquire directly.
         pInst->mpSalYieldMutex->acquire();
+    }
 }
 
 bool ImplSalYieldMutexTryToAcquire()
@@ -576,8 +543,6 @@ WinSalInstance::WinSalInstance()
 {
     mhComWnd                 = 0;
     mpSalYieldMutex          = new SalYieldMutex( this );
-    mpSalWaitMutex           = new osl::Mutex;
-    mnYieldWaitCount         = 0;
     mpSalYieldMutex->acquire();
     ::comphelper::SolarMutex::setSolarMutex( mpSalYieldMutex );
 }
@@ -587,7 +552,6 @@ WinSalInstance::~WinSalInstance()
     ::comphelper::SolarMutex::setSolarMutex( 0 );
     mpSalYieldMutex->release();
     delete mpSalYieldMutex;
-    delete mpSalWaitMutex;
     DestroyWindow( mhComWnd );
 }
 
@@ -706,7 +670,7 @@ SalYieldResult WinSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents,
     return eDidWork;
 }
 
-LRESULT CALLBACK SalComWndProc( HWND hWnd, UINT nMsg, WPARAM wParam, LPARAM lParam, int& rDef )
+LRESULT CALLBACK SalComWndProc( HWND, UINT nMsg, WPARAM wParam, LPARAM lParam, int& rDef )
 {
     LRESULT nRet = 0;
 
@@ -716,19 +680,6 @@ LRESULT CALLBACK SalComWndProc( HWND hWnd, UINT nMsg, WPARAM wParam, LPARAM lPar
             ImplSalYield( (bool)wParam, (bool)lParam );
             rDef = FALSE;
             break;
-        // If we get this message, because another GetMessage() call
-        // has received this message, we must post this message to
-        // us again, because in the other case we wait forever.
-        case SAL_MSG_RELEASEWAITYIELD:
-            {
-            WinSalInstance* pInst = GetSalData()->mpFirstInstance;
-            // this test does not need mpSalWaitMutex locked because
-            // it can only happen on the main thread
-            if ( pInst && pInst->mnYieldWaitCount )
-                PostMessageW( hWnd, SAL_MSG_RELEASEWAITYIELD, wParam, lParam );
-            }
-            rDef = FALSE;
-            break;
         case SAL_MSG_STARTTIMER:
             ImplSalStartTimer( (sal_uLong) lParam, FALSE );
             rDef = FALSE;