[Libreoffice-commits] core.git: vcl/win

[Stephan Bergmann (via logerrit)]

 vcl/win/app/salinst.cxx |   34 ++++------------------------------
 1 file changed, 4 insertions(+), 30 deletions(-)

New commits:
commit e9a16702ba025ca340bcded4fda37235d22410a1
Author:     Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Wed Sep 23 15:38:27 2020 +0200
Commit:     Stephan Bergmann <sbergman at redhat.com>
CommitDate: Wed Sep 23 17:07:32 2020 +0200

    Directly acquire m_aMutex, instead of looping on m_condition.wait()
    
    The code had been introduced with 9c9970952b0adec4a8c6de9a4cd54d0980cd47ec
    "tdf#96887 enhance SolarMutex AcquireWithWait for Windows", which mentions
    MsgWaitForMultipleObjects in multiple comments as if it had intended to make use
    of it somewhere, but no such use can be found in that commit.
    
    (I had come across this code when on Windows some CppunitTest had
    deadlocked for me during DeInitVCL when re-acquiring the released SolarMutex at
    the end of
    
    > #if defined _WIN32
    >     // See GetSystemClipboard (vcl/source/treelist/transfer2.cxx):
    >     if (auto const comp = css::uno::Reference<css::lang::XComponent>(
    >             pSVData->m_xSystemClipboard, css::uno::UNO_QUERY))
    >     {
    >         SolarMutexReleaser r; // unblock pending "clipboard content changed" notifications
    >         comp->dispose(); // will use CWinClipbImpl::s_aMutex
    >     }
    > #endif
    
    at vcl/source/app/svmain.cxx:490--498, and SalYieldMutex::doAcquire was waiting
    in m_condition.wait() for an m_condition.set() from some other thread that would
    never come---there were not even any other relevant threads left.  At first I
    thought this might be an issue with broken-by-design osl::Condition, where some
    other, meanwhile gone thread's m_condition.set() had been cancelled by this
    thread's m_condition.reset() in SalYieldMutex::doAcquire, but then its
    m_aMutex.tryToAcquire() should probably have succeeded so that it wouldn't have
    ended up in m_codition.wait().  But even if the use of m_condition might not be
    broken after all and the deadlock I observed would be caused by something else,
    it nevertheless looks pointless, so better clean it up.)
    
    Change-Id: Ia6c1aa133c14e19224d0f24c810f43363cb5898c
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103253
    Tested-by: Jenkins
    Reviewed-by: Stephan Bergmann <sbergman at redhat.com>

diff --git a/vcl/win/app/salinst.cxx b/vcl/win/app/salinst.cxx
index 5ebb22226eff..3af78712b1d2 100644
--- a/vcl/win/app/salinst.cxx
+++ b/vcl/win/app/salinst.cxx
@@ -99,9 +99,6 @@ static LRESULT CALLBACK SalComWndProcW( HWND hWnd, UINT nMsg, WPARAM wParam, LPA
 
 class SalYieldMutex : public comphelper::SolarMutex
 {
-public: // for ImplSalYield() and ImplSalYieldMutexAcquireWithWait()
-    osl::Condition            m_condition; /// for MsgWaitForMultipleObjects()
-
 protected:
     virtual void              doAcquire( sal_uInt32 nLockCount ) override;
     virtual sal_uInt32        doRelease( bool bUnlockAll ) override;
@@ -138,30 +135,11 @@ void SalYieldMutex::BeforeReleaseHandler()
 void SalYieldMutex::doAcquire( sal_uInt32 nLockCount )
 {
     WinSalInstance* pInst = GetSalData()->mpInstance;
-    if ( pInst && pInst->IsMainThread() )
+    if ( pInst && pInst->IsMainThread() && pInst->m_nNoYieldLock )
     {
-        if ( pInst->m_nNoYieldLock )
-            return;
-        // tdf#96887 If this is the main thread, then we must wait for two things:
-        // - the yield mutex being unlocked
-        // - 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.
-        do {
-            // reset condition *before* acquiring!
-            m_condition.reset();
-            if (m_aMutex.tryToAcquire())
-                break;
-            // wait for SalYieldMutex::release() to set the condition
-            osl::Condition::Result res = m_condition.wait();
-            assert(osl::Condition::Result::result_ok == res);
-        }
-        while ( true );
+        return;
     }
-    else
-        m_aMutex.acquire();
+    m_aMutex.acquire();
     ++m_nCount;
     --nLockCount;
 
@@ -174,11 +152,7 @@ sal_uInt32 SalYieldMutex::doRelease( const bool bUnlockAll )
     if ( pInst && pInst->m_nNoYieldLock && pInst->IsMainThread() )
         return 1;
 
-    sal_uInt32 nCount = comphelper::SolarMutex::doRelease( bUnlockAll );
-    // wake up ImplSalYieldMutexAcquireWithWait() after release
-    if ( 0 == m_nCount )
-        m_condition.set();
-    return nCount;
+    return comphelper::SolarMutex::doRelease( bUnlockAll );
 }
 
 bool SalYieldMutex::tryToAcquire()