[Libreoffice-commits] core.git: Branch 'feature/fixes24' - 2 commits - vcl/inc vcl/win
Michael Stahl
mstahl at redhat.com
Tue Jun 14 08:58:27 UTC 2016
vcl/inc/win/saldata.hxx | 4 -
vcl/inc/win/salinst.h | 9 --
vcl/win/source/app/salinst.cxx | 121 ++++++++++++----------------------------
vcl/win/source/app/saltimer.cxx | 38 +++++++-----
4 files changed, 65 insertions(+), 107 deletions(-)
New commits:
commit e7b945e49c16f1c104ed048a5c12e7846628cebb
Author: Michael Stahl <mstahl at redhat.com>
Date: Mon Apr 11 23:49:12 2016 +0200
tdf#96887 vcl: stop using periodic timers on WNT
Every time the periodic timer fires, it does a PostMessage() to the
main thread. The main thread will only process the first message and
discard the rest anyway, but with a short enough timer and other
threads hogging the SolarMutex it's possible that the message queue
overflows and other PostMessage calls fail with ERROR_NOT_ENOUGH_QUOTA.
Try to avoid the problem by having the WinSalTimer always be a one-shot
timer; when it fires and the main thread processes the posted message,
it is restarted with the new due time.
This requires creating a new TimerQueueTimer because
ChangeTimerQueueTimer only works on periodic timers.
Change-Id: I816bd3fa5fbfbea4f26be8ff680a1c916618d3f9
Reviewed-on: https://gerrit.libreoffice.org/24024
Tested-by: Jenkins <ci at libreoffice.org>
Reviewed-by: Jan Holesovsky <kendy at collabora.com>
Reviewed-by: Michael Stahl <mstahl at redhat.com>
diff --git a/vcl/inc/win/saldata.hxx b/vcl/inc/win/saldata.hxx
index 66b8a7f..83e853f 100644
--- a/vcl/inc/win/saldata.hxx
+++ b/vcl/inc/win/saldata.hxx
@@ -277,6 +277,8 @@ int ImplSalWICompareAscii( const wchar_t* pStr1, const char* pStr2 );
// Call the Timer's callback from the main thread
#define SAL_MSG_TIMER_CALLBACK (WM_USER+162)
+// Stop the timer from the main thread; wParam = 0, lParam = 0
+#define SAL_MSG_STOPTIMER (WM_USER+163)
inline void SetWindowPtr( HWND hWnd, WinSalFrame* pThis )
{
diff --git a/vcl/inc/win/salinst.h b/vcl/inc/win/salinst.h
index fe2c341..43c0313 100644
--- a/vcl/inc/win/salinst.h
+++ b/vcl/inc/win/salinst.h
@@ -83,6 +83,7 @@ SalFrame* ImplSalCreateFrame( WinSalInstance* pInst, HWND hWndParent, SalFrameSt
SalObject* ImplSalCreateObject( WinSalInstance* pInst, WinSalFrame* pParent );
HWND ImplSalReCreateHWND( HWND hWndParent, HWND oldhWnd, bool bAsChild );
void ImplSalStartTimer( sal_uIntPtr nMS, bool bMutex = false );
+void ImplSalStopTimer();
#endif // INCLUDED_VCL_INC_WIN_SALINST_H
diff --git a/vcl/win/source/app/salinst.cxx b/vcl/win/source/app/salinst.cxx
index c76fa44..b229d46 100644
--- a/vcl/win/source/app/salinst.cxx
+++ b/vcl/win/source/app/salinst.cxx
@@ -689,6 +689,10 @@ LRESULT CALLBACK SalComWndProc( HWND, UINT nMsg, WPARAM wParam, LPARAM lParam, i
ImplSalStartTimer( (sal_uLong) lParam, FALSE );
rDef = FALSE;
break;
+ case SAL_MSG_STOPTIMER:
+ ImplSalStopTimer();
+ rDef = FALSE;
+ break;
case SAL_MSG_CREATEFRAME:
nRet = (LRESULT)ImplSalCreateFrame( GetSalData()->mpFirstInstance, (HWND)lParam, (SalFrameStyleFlags)wParam );
rDef = FALSE;
diff --git a/vcl/win/source/app/saltimer.cxx b/vcl/win/source/app/saltimer.cxx
index c6f04be..641d2eb 100644
--- a/vcl/win/source/app/saltimer.cxx
+++ b/vcl/win/source/app/saltimer.cxx
@@ -34,12 +34,22 @@ void CALLBACK SalTimerProc(PVOID pParameter, BOOLEAN bTimerOrWaitFired);
// See http://msdn.microsoft.com/en-us/library/windows/desktop/ms687003%28v=vs.85%29.aspx
// (and related pages) for details about the Timer Queues.
-void ImplSalStopTimer(SalData* pSalData)
+// in order to prevent concurrent execution of ImplSalStartTimer and double
+// deletion of timer (which is extremely likely, given that
+// INVALID_HANDLE_VALUE waits for the callback to run on the main thread),
+// this must run on the main thread too
+void ImplSalStopTimer()
{
+ SalData *const pSalData = GetSalData();
HANDLE hTimer = pSalData->mnTimerId;
- pSalData->mnTimerId = 0;
- DeleteTimerQueueTimer(NULL, hTimer, INVALID_HANDLE_VALUE);
+ if (hTimer)
+ {
+ pSalData->mnTimerId = 0; // reset so it doesn't restart
+ DeleteTimerQueueTimer(NULL, hTimer, INVALID_HANDLE_VALUE);
+ pSalData->mnNextTimerTime = 0;
+ }
MSG aMsg;
+ // this needs to run on the main thread
while (PeekMessageW(&aMsg, 0, SAL_MSG_TIMER_CALLBACK, SAL_MSG_TIMER_CALLBACK, PM_REMOVE))
{
// just remove all the SAL_MSG_TIMER_CALLBACKs
@@ -61,11 +71,13 @@ void ImplSalStartTimer( sal_uLong nMS, bool bMutex )
if (nMS > MAX_SYSPERIOD)
nMS = MAX_SYSPERIOD;
- // change if it exists, create if not
+ // cannot change a one-shot timer, so delete it and create new one
if (pSalData->mnTimerId)
- ChangeTimerQueueTimer(NULL, pSalData->mnTimerId, nMS, nMS);
- else
- CreateTimerQueueTimer(&pSalData->mnTimerId, NULL, SalTimerProc, NULL, nMS, nMS, WT_EXECUTEINTIMERTHREAD);
+ {
+ DeleteTimerQueueTimer(NULL, pSalData->mnTimerId, INVALID_HANDLE_VALUE);
+ pSalData->mnTimerId = 0;
+ }
+ CreateTimerQueueTimer(&pSalData->mnTimerId, NULL, SalTimerProc, NULL, nMS, 0, WT_EXECUTEINTIMERTHREAD);
pSalData->mnNextTimerTime = pSalData->mnLastEventTime + nMS;
}
@@ -93,12 +105,8 @@ void WinSalTimer::Stop()
{
SalData* pSalData = GetSalData();
- // If we have a timer, than
- if ( pSalData->mnTimerId )
- {
- ImplSalStopTimer(pSalData);
- pSalData->mnNextTimerTime = 0;
- }
+ assert(pSalData->mpFirstInstance);
+ SendMessageW(pSalData->mpFirstInstance->mhComWnd, SAL_MSG_STOPTIMER, 0, 0);
}
/** This gets invoked from a Timer Queue thread.
@@ -158,9 +166,11 @@ void EmitTimerCallback()
pSVData->mpSalTimer->CallCallback( idle );
ImplSalYieldMutexRelease();
+ // Run the timer again if it was started before, and also
// Run the timer in the correct time, if we started this
// with a small timeout, because we didn't get the mutex
- if (pSalData->mnTimerId && (pSalData->mnTimerMS != pSalData->mnTimerOrgMS))
+ // - but not if mnTimerId is 0, which is set by ImplSalStopTimer()
+ if (pSalData->mnTimerId)
ImplSalStartTimer(pSalData->mnTimerOrgMS, false);
}
else
commit 7eb7fe28892db7ef9452c1a2eef945064dbe355f
Author: Armin Le Grand <Armin.Le.Grand at cib.de>
Date: Fri Apr 8 15:14:43 2016 +0200
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 at libreoffice.org>
Reviewed-by: Armin Le Grand <Armin.Le.Grand at cib.de>
diff --git a/vcl/inc/win/saldata.hxx b/vcl/inc/win/saldata.hxx
index ed8a2fd..66b8a7f 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 b6408ea..fe2c341 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/source/app/salinst.cxx b/vcl/win/source/app/salinst.cxx
index 40e44d2..c76fa44 100644
--- a/vcl/win/source/app/salinst.cxx
+++ b/vcl/win/source/app/salinst.cxx
@@ -111,9 +111,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;
@@ -149,10 +149,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();
@@ -160,31 +161,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()
}
}
}
@@ -218,57 +209,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()
@@ -578,8 +545,6 @@ WinSalInstance::WinSalInstance()
{
mhComWnd = 0;
mpSalYieldMutex = new SalYieldMutex( this );
- mpSalWaitMutex = new osl::Mutex;
- mnYieldWaitCount = 0;
mpSalYieldMutex->acquire();
::comphelper::SolarMutex::setSolarMutex( mpSalYieldMutex );
}
@@ -589,7 +554,6 @@ WinSalInstance::~WinSalInstance()
::comphelper::SolarMutex::setSolarMutex( 0 );
mpSalYieldMutex->release();
delete mpSalYieldMutex;
- delete mpSalWaitMutex;
DestroyWindow( mhComWnd );
}
@@ -711,7 +675,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;
@@ -721,19 +685,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;
More information about the Libreoffice-commits
mailing list