vcl/win/window/salframe.cxx | 48 ++--
winaccessibility/inc/AccEventListener.hxx | 2
winaccessibility/inc/AccObject.hxx | 4
winaccessibility/inc/AccObjectWinManager.hxx | 9
winaccessibility/source/service/AccEventListener.cxx | 9
winaccessibility/source/service/AccObject.cxx | 9
winaccessibility/source/service/AccObjectWinManager.cxx | 165 +++++++++-------
winaccessibility/source/service/msaaservice_impl.cxx | 2
8 files changed, 150 insertions(+), 98 deletions(-)
New commits:
commit 2b317048d72d39a9d64b2eb8055eef7045ad449a
Author: Michael Stahl <michael.st...@allotropia.de>
AuthorDate: Tue Jun 13 12:30:44 2023 +0200
Commit: Michael Stahl <michael.st...@allotropia.de>
CommitDate: Tue Jun 13 12:30:44 2023 +0200
tdf#155794 vcl: handle WM_GETOBJECT without SolarMutex
SalFrameWndProc() handles WM_GETOBJECT by acquiring SolarMutex and
calling ImplHandleGetObject(), which again acquires the SolarMutex
inside Application::SetSettings().
This was introduced with commit db214684057e3ff2fa32d57c00507309dd6c24d6
due to thread-safety crashes but it turns out that it can be problematic.
When loading a document on a non-main thread, WinSalFrame::SetTitle()
calls SetWindowTextW which is equivalent to SendMessage(WM_SETTEXT),
while holding SolarMutex, and if the main thread doesn't finish
processing it then that's a deadlock.
Typically Desktop::Main() has already created the mxAccessBridge, so
ImplHandleGetObject() most likely doesn't need to do it, so just skip
the Settings code there in case the SolarMutex is locked by another
thread.
In case the SolarMutex is locked by another thread, do an unsafe read of
ImplGetSVData()->mxAccessBridge - this should work until ImplSVData is
deleted, by which time no Windows should exist anymore that could be
receiving messages.
This fixes part of the problem, winaccessibility also needs to stop
using SolarMutex.
Change-Id: I62b027ad06d2c3eb06a5f64b052a4acd0908f79c
diff --git a/vcl/win/window/salframe.cxx b/vcl/win/window/salframe.cxx
index 4fb572fc19a9..ab6db6aa41b3 100644
--- a/vcl/win/window/salframe.cxx
+++ b/vcl/win/window/salframe.cxx
@@ -5280,27 +5280,40 @@ static void ImplHandleIMENotify( HWND hWnd, WPARAM
wParam )
static bool
ImplHandleGetObject(HWND hWnd, LPARAM lParam, WPARAM wParam, LRESULT & nRet)
{
- // IA2 should be enabled automatically
- AllSettings aSettings = Application::GetSettings();
- MiscSettings aMisc = aSettings.GetMiscSettings();
- aMisc.SetEnableATToolSupport( true );
- aSettings.SetMiscSettings( aMisc );
- Application::SetSettings( aSettings );
+ uno::Reference<accessibility::XMSAAService> xMSAA;
+ if (ImplSalYieldMutexTryToAcquire())
+ {
+ // IA2 should be enabled automatically
+ AllSettings aSettings = Application::GetSettings();
+ MiscSettings aMisc = aSettings.GetMiscSettings();
+ aMisc.SetEnableATToolSupport( true );
+ aSettings.SetMiscSettings( aMisc );
+ Application::SetSettings( aSettings );
- if (!Application::GetSettings().GetMiscSettings().GetEnableATToolSupport())
- return false; // locked down somehow ?
+ if
(!Application::GetSettings().GetMiscSettings().GetEnableATToolSupport())
+ return false; // locked down somehow ?
- ImplSVData* pSVData = ImplGetSVData();
+ ImplSVData* pSVData = ImplGetSVData();
- // Make sure to launch Accessibility only the following criteria are
satisfied
- // to avoid RFT interrupts regular accessibility processing
- if ( !pSVData->mxAccessBridge.is() )
- {
- if( !InitAccessBridge() )
- return false;
+ // Make sure to launch Accessibility only the following criteria are
satisfied
+ // to avoid RFT interrupts regular accessibility processing
+ if ( !pSVData->mxAccessBridge.is() )
+ {
+ if( !InitAccessBridge() )
+ return false;
+ }
+ xMSAA.set(pSVData->mxAccessBridge, uno::UNO_QUERY);
+ ImplSalYieldMutexRelease();
+ }
+ else
+ { // tdf#155794: access without locking: hopefully this should be fine
+ // as the bridge is typically inited in Desktop::Main() already and the
+ // WM_GETOBJECT is received only on the main thread and by the time in
+ // VCL shutdown when ImplSvData dies there should not be Windows any
+ // more that could receive messages.
+ xMSAA.set(ImplGetSVData()->mxAccessBridge, uno::UNO_QUERY);
}
- uno::Reference< accessibility::XMSAAService > xMSAA(
pSVData->mxAccessBridge, uno::UNO_QUERY );
if ( xMSAA.is() )
{
sal_Int32 lParam32 = static_cast<sal_Int32>(lParam);
@@ -5831,12 +5844,11 @@ static LRESULT CALLBACK SalFrameWndProc( HWND hWnd,
UINT nMsg, WPARAM wParam, LP
break;
case WM_GETOBJECT:
- ImplSalYieldMutexAcquireWithWait();
+ // tdf#155794: this must complete without taking SolarMutex
if ( ImplHandleGetObject( hWnd, lParam, wParam, nRet ) )
{
rDef = false;
}
- ImplSalYieldMutexRelease();
break;
case WM_APPCOMMAND:
commit af1e9f9b8726b84dc5c6e644ed317873dacbe1f5
Author: Michael Stahl <michael.st...@allotropia.de>
AuthorDate: Mon Jun 12 20:03:14 2023 +0200
Commit: Michael Stahl <michael.st...@allotropia.de>
CommitDate: Mon Jun 12 20:03:14 2023 +0200
tdf#155794 winaccessibility: no SolarMutex in getAccObjectPtr()
MSAAServiceImpl::getAccObjectPtr() is called when processing
WM_GETOBJECT messages, and this can happen (at least when NVDA is
active) during processing SendMessages.
When loading a document on a non-main thread, WinSalFrame::SetTitle()
calls SetWindowTextW which is equivalent to SendMessage(WM_SETTEXT),
while holding SolarMutex, and if the main thread doesn't finish
processing it then that's a deadlock.
Introduce a new mutex in AccObjectWinManager and use it to guard the 2
members that getAccObjectPtr() reads, while keeping the rest of
winaccessibility with the SolarMutex, as the UNO services may be called
on any thread.
This fixes part of the problem, VCL also needs to stop using SolarMutex.
Change-Id: I6df5889fd76f59146b4b0b1e5f4513232f8ab867
diff --git a/winaccessibility/inc/AccEventListener.hxx
b/winaccessibility/inc/AccEventListener.hxx
index 9c5515529440..095f1cad1598 100644
--- a/winaccessibility/inc/AccEventListener.hxx
+++ b/winaccessibility/inc/AccEventListener.hxx
@@ -75,7 +75,7 @@ public:
//get the accessible parent's role
virtual short GetParentRole();
- void RemoveMeFromBroadcaster();
+ void RemoveMeFromBroadcaster(bool isNotifyDestroy);
};
#endif // INCLUDED_WINACCESSIBILITY_INC_ACCEVENTLISTENER_HXX
diff --git a/winaccessibility/inc/AccObject.hxx
b/winaccessibility/inc/AccObject.hxx
index 2a024642d8ac..6211b2a48d3e 100644
--- a/winaccessibility/inc/AccObject.hxx
+++ b/winaccessibility/inc/AccObject.hxx
@@ -61,7 +61,7 @@ private:
long m_resID;
HWND m_pParantID;
bool m_bShouldDestroy; //avoid access COM interface when
acc object is deleted
- IMAccessible* m_pIMAcc;
+ IMAccessible* const m_pIMAcc; //
AccObjectManager::GetTopWindowIMAccessible relies on this being const
AccObject* m_pParentObj;
IAccChildList m_childrenList;
::rtl::Reference<AccEventListener> m_pListener;
@@ -99,7 +99,7 @@ public:
void SetParentHWND(HWND hWnd);//need to set top window handle when send
event to AT
HWND GetParentHWND();
- void SetListener(::rtl::Reference<AccEventListener> const& pListener);
+ ::rtl::Reference<AccEventListener>
SetListener(::rtl::Reference<AccEventListener> const& pListener);
AccEventListener* getListener();
void SetParentObj(AccObject* pParentAccObj);
diff --git a/winaccessibility/inc/AccObjectWinManager.hxx
b/winaccessibility/inc/AccObjectWinManager.hxx
index 1529bc40ec7f..a533c1be373d 100644
--- a/winaccessibility/inc/AccObjectWinManager.hxx
+++ b/winaccessibility/inc/AccObjectWinManager.hxx
@@ -22,6 +22,8 @@
#include <com/sun/star/accessibility/XAccessible.hpp>
#include <map>
+#include <mutex>
+
#if !defined WIN32_LEAN_AND_MEAN
# define WIN32_LEAN_AND_MEAN
#endif
@@ -63,6 +65,9 @@ private:
typedef std::map<const HWND, css::accessibility::XAccessible* >
XHWNDToDocumentHash;
+ // guard any access to XIdAccList and HwndXAcc
+ std::recursive_mutex m_Mutex;
+
//XAccessible to AccObject
XIdToAccObjHash XIdAccList;
@@ -86,11 +91,11 @@ private:
long ImpleGenerateResID();
AccObject* GetAccObjByXAcc( css::accessibility::XAccessible* pXAcc);
- AccObject* GetTopWindowAccObj(HWND hWnd);
+ IMAccessible* GetTopWindowIMAccessible(HWND hWnd);
css::accessibility::XAccessible* GetAccDocByHWND(HWND hWnd);
- static void DeleteAccListener( AccObject* pAccObj );
+ static rtl::Reference<AccEventListener> DeleteAccListener(AccObject*
pAccObj);
static void InsertAccChildNode(AccObject* pCurObj,AccObject*
pParentObj,HWND pWnd);
static void DeleteAccChildNode(AccObject* pChild);
void DeleteFromHwndXAcc(css::accessibility::XAccessible const *
pXAcc );
diff --git a/winaccessibility/source/service/AccEventListener.cxx
b/winaccessibility/source/service/AccEventListener.cxx
index e2be5cce145d..0a55d481c0ae 100644
--- a/winaccessibility/source/service/AccEventListener.cxx
+++ b/winaccessibility/source/service/AccEventListener.cxx
@@ -223,7 +223,7 @@ short AccEventListener::GetParentRole()
/**
* remove the listener from accessible object
*/
-void AccEventListener::RemoveMeFromBroadcaster()
+void AccEventListener::RemoveMeFromBroadcaster(bool const isNotifyDestroy)
{
try
{
@@ -244,7 +244,10 @@ void AccEventListener::RemoveMeFromBroadcaster()
catch (Exception const&)
{ // may throw if it's already disposed - ignore that
}
- pAgent->NotifyDestroy(m_xAccessible.get());
+ if (isNotifyDestroy)
+ {
+ pAgent->NotifyDestroy(m_xAccessible.get());
+ }
m_xAccessible.clear(); // release cyclic reference
}
catch(...)
@@ -261,7 +264,7 @@ void AccEventListener::disposing( const
css::lang::EventObject& /*Source*/ )
{
SolarMutexGuard g;
- RemoveMeFromBroadcaster();
+ RemoveMeFromBroadcaster(true);
}
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/winaccessibility/source/service/AccObject.cxx
b/winaccessibility/source/service/AccObject.cxx
index b668dac77268..c606970c753d 100644
--- a/winaccessibility/source/service/AccObject.cxx
+++ b/winaccessibility/source/service/AccObject.cxx
@@ -162,7 +162,7 @@ AccObject::AccObject(XAccessible* pAcc,
AccObjectManagerAgent* pAgent,
m_resID (NULL),
m_pParantID (nullptr),
m_bShouldDestroy(false),
- m_pIMAcc (nullptr),
+ m_pIMAcc (UAccCOMCreateInstance()),
m_pParentObj(nullptr),
m_pListener (pListener),
m_xAccRef( pAcc )
@@ -186,7 +186,6 @@ AccObject::AccObject(XAccessible* pAcc,
AccObjectManagerAgent* pAgent,
*/
AccObject::~AccObject()
{
- m_pIMAcc = nullptr;
m_xAccRef = nullptr;
m_xAccActionRef = nullptr;
m_xAccContextRef = nullptr;
@@ -256,8 +255,6 @@ void AccObject::UpdateValidWindow()
*/
void AccObject::ImplInitializeCreateObj()
{
- m_pIMAcc = UAccCOMCreateInstance();
-
assert(m_pIMAcc);
}
@@ -1162,9 +1159,11 @@ void AccObject::SetParentHWND(HWND hWnd)
m_pParantID = hWnd;
}
-void AccObject::SetListener(rtl::Reference<AccEventListener> const& pListener)
+rtl::Reference<AccEventListener>
AccObject::SetListener(rtl::Reference<AccEventListener> const& pListener)
{
+ rtl::Reference<AccEventListener> pRet(m_pListener);
m_pListener = pListener;
+ return pRet;
}
AccEventListener* AccObject::getListener()
diff --git a/winaccessibility/source/service/AccObjectWinManager.cxx
b/winaccessibility/source/service/AccObjectWinManager.cxx
index b56db56d4bab..a29c3a2513cd 100644
--- a/winaccessibility/source/service/AccObjectWinManager.cxx
+++ b/winaccessibility/source/service/AccObjectWinManager.cxx
@@ -75,8 +75,12 @@ AccObjectWinManager::AccObjectWinManager(
AccObjectManagerAgent* Agent ):
*/
AccObjectWinManager::~AccObjectWinManager()
{
- XIdAccList.clear();
- HwndXAcc.clear();
+ {
+ std::scoped_lock l(m_Mutex);
+
+ XIdAccList.clear();
+ HwndXAcc.clear();
+ }
XResIdAccList.clear();
XHWNDDocList.clear();
#ifdef ACC_DEBUG
@@ -100,13 +104,7 @@ AccObjectWinManager::Get_ToATInterface(HWND hWnd, long
lParam, WPARAM wParam)
if(lParam == OBJID_CLIENT )
{
- AccObject* topWindowAccObj = GetTopWindowAccObj(hWnd);
- if(topWindowAccObj)
- {
- pRetIMAcc = topWindowAccObj->GetIMAccessible();
- if(pRetIMAcc)
- pRetIMAcc->AddRef();//increase COM reference count
- }
+ pRetIMAcc = GetTopWindowIMAccessible(hWnd);
}
if ( pRetIMAcc && lParam == OBJID_CLIENT )
@@ -128,6 +126,8 @@ AccObject* AccObjectWinManager::GetAccObjByXAcc(
XAccessible* pXAcc)
if( pXAcc == nullptr)
return nullptr;
+ std::scoped_lock l(m_Mutex);
+
XIdToAccObjHash::iterator pIndTemp = XIdAccList.find( pXAcc );
if ( pIndTemp == XIdAccList.end() )
return nullptr;
@@ -140,13 +140,26 @@ AccObject* AccObjectWinManager::GetAccObjByXAcc(
XAccessible* pXAcc)
* @param hWnd, top window handle
* @return pointer to AccObject
*/
-AccObject* AccObjectWinManager::GetTopWindowAccObj(HWND hWnd)
+IMAccessible * AccObjectWinManager::GetTopWindowIMAccessible(HWND hWnd)
{
+ std::scoped_lock l(m_Mutex); // tdf#155794 for HwndXAcc and XIdAccList
+
XHWNDToXAccHash::iterator iterResult =HwndXAcc.find(hWnd);
if(iterResult == HwndXAcc.end())
return nullptr;
XAccessible* pXAcc = static_cast<XAccessible*>(iterResult->second);
- return GetAccObjByXAcc(pXAcc);
+ AccObject *const pAccObject(GetAccObjByXAcc(pXAcc));
+ if (!pAccObject)
+ {
+ return nullptr;
+ }
+ IMAccessible *const pRet(pAccObject->GetIMAccessible());
+ if (!pRet)
+ {
+ return nullptr;
+ }
+ pRet->AddRef();
+ return pRet;
}
/**
@@ -471,6 +484,8 @@ void AccObjectWinManager::DeleteAccChildNode( AccObject*
pObj )
*/
void AccObjectWinManager::DeleteFromHwndXAcc(XAccessible const * pXAcc )
{
+ std::scoped_lock l(m_Mutex);
+
auto iter = std::find_if(HwndXAcc.begin(), HwndXAcc.end(),
[&pXAcc](XHWNDToXAccHash::value_type& rEntry) { return rEntry.second
== pXAcc; });
if (iter != HwndXAcc.end())
@@ -513,34 +528,46 @@ void AccObjectWinManager::DeleteAccObj( XAccessible*
pXAcc )
{
if( pXAcc == nullptr )
return;
- XIdToAccObjHash::iterator temp = XIdAccList.find(pXAcc);
- if( temp != XIdAccList.end() )
- {
- ResIdGen.SetSub( temp->second.GetResID() );
- }
- else
- {
- return;
- }
- AccObject& accObj = temp->second;
- DeleteAccChildNode( &accObj );
- DeleteAccListener( &accObj );
- if( accObj.GetIMAccessible() )
+ rtl::Reference<AccEventListener> pListener;
+
{
- accObj.GetIMAccessible()->Release();
+ std::scoped_lock l(m_Mutex);
+
+ XIdToAccObjHash::iterator temp = XIdAccList.find(pXAcc);
+ if( temp != XIdAccList.end() )
+ {
+ ResIdGen.SetSub( temp->second.GetResID() );
+ }
+ else
+ {
+ return;
+ }
+
+ AccObject& accObj = temp->second;
+ DeleteAccChildNode( &accObj );
+ pListener = DeleteAccListener(&accObj);
+ accObj.NotifyDestroy(true);
+ if( accObj.GetIMAccessible() )
+ {
+ accObj.GetIMAccessible()->Release();
+ }
+ size_t i = XResIdAccList.erase(accObj.GetResID());
+ assert(i != 0);
+ DeleteFromHwndXAcc(pXAcc);
+ if( accObj.GetRole() == DOCUMENT ||
+ accObj.GetRole() == DOCUMENT_PRESENTATION ||
+ accObj.GetRole() == DOCUMENT_SPREADSHEET ||
+ accObj.GetRole() == DOCUMENT_TEXT )
+ {
+ XHWNDDocList.erase(accObj.GetParentHWND());
+ }
+ XIdAccList.erase(pXAcc); // note: this invalidates accObj so do it
last!
}
- size_t i = XResIdAccList.erase(accObj.GetResID());
- assert(i != 0);
- DeleteFromHwndXAcc(pXAcc);
- if( accObj.GetRole() == DOCUMENT ||
- accObj.GetRole() == DOCUMENT_PRESENTATION ||
- accObj.GetRole() == DOCUMENT_SPREADSHEET ||
- accObj.GetRole() == DOCUMENT_TEXT )
+ if (pListener)
{
- XHWNDDocList.erase(accObj.GetParentHWND());
+ pListener->RemoveMeFromBroadcaster(false);
}
- XIdAccList.erase(pXAcc); // note: this invalidates accObj so do it last!
}
/**
@@ -548,13 +575,9 @@ void AccObjectWinManager::DeleteAccObj( XAccessible* pXAcc
)
* @param pAccObj Accobject pointer.
* @return
*/
-void AccObjectWinManager::DeleteAccListener( AccObject* pAccObj )
+rtl::Reference<AccEventListener> AccObjectWinManager::DeleteAccListener(
AccObject* pAccObj )
{
- AccEventListener* listener = pAccObj->getListener();
- if( listener==nullptr )
- return;
- listener->RemoveMeFromBroadcaster();
- pAccObj->SetListener(nullptr);
+ return pAccObj->SetListener(nullptr);
}
/**
@@ -647,29 +670,6 @@ void AccObjectWinManager::InsertAccChildNode( AccObject*
pCurObj, AccObject* pPa
*/
bool AccObjectWinManager::InsertAccObj( XAccessible* pXAcc,XAccessible*
pParentXAcc,HWND pWnd )
{
- XIdToAccObjHash::iterator itXacc = XIdAccList.find( pXAcc );
- if (itXacc != XIdAccList.end() )
- {
- short nCurRole =GetRole(pXAcc);
- if (AccessibleRole::SHAPE == nCurRole)
- {
- AccObject &objXacc = itXacc->second;
- AccObject *pObjParent = objXacc.GetParentObj();
- if (pObjParent &&
- pObjParent->GetXAccessible().is() &&
- pObjParent->GetXAccessible().get() != pParentXAcc)
- {
- XIdToAccObjHash::iterator itXaccParent = XIdAccList.find(
pParentXAcc );
- if(itXaccParent != XIdAccList.end())
- {
- objXacc.SetParentObj(&(itXaccParent->second));
- }
- }
- }
- return false;
- }
-
-
Reference< XAccessibleContext > pRContext;
if( pXAcc == nullptr)
@@ -679,6 +679,33 @@ bool AccObjectWinManager::InsertAccObj( XAccessible*
pXAcc,XAccessible* pParentX
if( !pRContext.is() )
return false;
+ {
+ short nCurRole = GetRole(pXAcc);
+
+ std::scoped_lock l(m_Mutex);
+
+ XIdToAccObjHash::iterator itXacc = XIdAccList.find( pXAcc );
+ if (itXacc != XIdAccList.end() )
+ {
+ if (AccessibleRole::SHAPE == nCurRole)
+ {
+ AccObject &objXacc = itXacc->second;
+ AccObject *pObjParent = objXacc.GetParentObj();
+ if (pObjParent &&
+ pObjParent->GetXAccessible().is() &&
+ pObjParent->GetXAccessible().get() != pParentXAcc)
+ {
+ XIdToAccObjHash::iterator itXaccParent = XIdAccList.find(
pParentXAcc );
+ if(itXaccParent != XIdAccList.end())
+ {
+ objXacc.SetParentObj(&(itXaccParent->second));
+ }
+ }
+ }
+ return false;
+ }
+ }
+
if( pWnd == nullptr )
{
if(pParentXAcc)
@@ -726,9 +753,13 @@ bool AccObjectWinManager::InsertAccObj( XAccessible*
pXAcc,XAccessible* pParentX
else
return false;
- XIdAccList.emplace(pXAcc, pObj);
- XIdToAccObjHash::iterator pIndTemp = XIdAccList.find( pXAcc );
- XResIdAccList.emplace(pObj.GetResID(),&(pIndTemp->second));
+ {
+ std::scoped_lock l(m_Mutex);
+
+ XIdAccList.emplace(pXAcc, pObj);
+ XIdToAccObjHash::iterator pIndTemp = XIdAccList.find( pXAcc );
+ XResIdAccList.emplace(pObj.GetResID(),&(pIndTemp->second));
+ }
AccObject* pCurObj = GetAccObjByXAcc(pXAcc);
if( pCurObj )
@@ -752,6 +783,8 @@ bool AccObjectWinManager::InsertAccObj( XAccessible*
pXAcc,XAccessible* pParentX
*/
void AccObjectWinManager::SaveTopWindowHandle(HWND hWnd,
css::accessibility::XAccessible* pXAcc)
{
+ std::scoped_lock l(m_Mutex);
+
HwndXAcc.emplace(hWnd,pXAcc);
}
diff --git a/winaccessibility/source/service/msaaservice_impl.cxx
b/winaccessibility/source/service/msaaservice_impl.cxx
index 15959e2b4c83..3c40c139be66 100644
--- a/winaccessibility/source/service/msaaservice_impl.cxx
+++ b/winaccessibility/source/service/msaaservice_impl.cxx
@@ -92,7 +92,7 @@ public:
sal_Int64 MSAAServiceImpl::getAccObjectPtr(
sal_Int64 hWnd, sal_Int64 lParam, sal_Int64 wParam)
{
- SolarMutexGuard g;
+ // tdf#155794: this must complete without taking SolarMutex
if (!m_pTopWindowListener.is())
{
