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 f9663d2d06054d3d7ceb8ff12bc550b2be6062ca Author: Michael Stahl <[email protected]> AuthorDate: Tue Jun 13 12:30:44 2023 +0200 Commit: Michael Stahl <[email protected]> CommitDate: Tue Sep 10 12:56:56 2024 +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 (cherry picked from commit 2b317048d72d39a9d64b2eb8055eef7045ad449a) diff --git a/vcl/win/window/salframe.cxx b/vcl/win/window/salframe.cxx index e972e9bc5684..3d9bafef9dbb 100644 --- a/vcl/win/window/salframe.cxx +++ b/vcl/win/window/salframe.cxx @@ -5266,27 +5266,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() ) { // mhOnSetTitleWnd not set to reasonable value anywhere... @@ -5823,12 +5836,11 @@ LRESULT CALLBACK SalFrameWndProc( HWND hWnd, UINT nMsg, WPARAM wParam, LPARAM lP break; case WM_GETOBJECT: - ImplSalYieldMutexAcquireWithWait(); + // tdf#155794: this must complete without taking SolarMutex if ( ImplHandleGetObject( hWnd, lParam, wParam, nRet ) ) { rDef = int(false); } - ImplSalYieldMutexRelease(); break; case WM_APPCOMMAND: commit ac64cf2d0644ece449343d3874213561409ed628 Author: Michael Stahl <[email protected]> AuthorDate: Mon Jun 12 20:03:14 2023 +0200 Commit: Michael Stahl <[email protected]> CommitDate: Tue Sep 10 12:51:46 2024 +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. (cherry picked from commit af1e9f9b8726b84dc5c6e644ed317873dacbe1f5) Change-Id: I6df5889fd76f59146b4b0b1e5f4513232f8ab867 diff --git a/winaccessibility/inc/AccEventListener.hxx b/winaccessibility/inc/AccEventListener.hxx index fc8f8f85c02d..e80502a3dedf 100644 --- a/winaccessibility/inc/AccEventListener.hxx +++ b/winaccessibility/inc/AccEventListener.hxx @@ -77,7 +77,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 89ae14b5e011..73a3ade6580a 100644 --- a/winaccessibility/inc/AccObject.hxx +++ b/winaccessibility/inc/AccObject.hxx @@ -58,7 +58,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; @@ -96,7 +96,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 ba04f217b436..e29e2bab8d1d 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> + #include <windows.h> #include <osl/mutex.hxx> #include <rtl/ref.hxx> @@ -61,6 +63,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; @@ -84,11 +89,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* pXAcc ); diff --git a/winaccessibility/source/service/AccEventListener.cxx b/winaccessibility/source/service/AccEventListener.cxx index c508de5092b1..613296dbda15 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 a80db3bd2d0b..48c356184dc6 100644 --- a/winaccessibility/source/service/AccObject.cxx +++ b/winaccessibility/source/service/AccObject.cxx @@ -160,7 +160,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 ) @@ -184,7 +184,6 @@ AccObject::AccObject(XAccessible* pAcc, AccObjectManagerAgent* pAgent, */ AccObject::~AccObject() { - m_pIMAcc = nullptr; m_xAccRef = nullptr; m_xAccActionRef = nullptr; m_xAccContextRef = nullptr; @@ -254,8 +253,6 @@ void AccObject::UpdateValidWindow() */ void AccObject::ImplInitializeCreateObj() { - m_pIMAcc = UAccCOMCreateInstance(); - assert(m_pIMAcc); } @@ -1168,9 +1165,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 2e97c62f2835..782f101d1db1 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 ) @@ -129,6 +127,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; @@ -141,13 +141,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; } /** @@ -475,6 +488,8 @@ void AccObjectWinManager::DeleteAccChildNode( AccObject* pObj ) */ void AccObjectWinManager::DeleteFromHwndXAcc(XAccessible* pXAcc ) { + std::scoped_lock l(m_Mutex); + XHWNDToXAccHash::iterator iter = HwndXAcc.begin(); while(iter!=HwndXAcc.end()) { @@ -523,34 +538,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! } /** @@ -558,13 +585,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); } /** @@ -657,29 +680,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) @@ -689,6 +689,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) @@ -736,9 +763,13 @@ bool AccObjectWinManager::InsertAccObj( XAccessible* pXAcc,XAccessible* pParentX else return false; - XIdAccList.insert( XIdToAccObjHash::value_type( pXAcc, pObj )); - XIdToAccObjHash::iterator pIndTemp = XIdAccList.find( pXAcc ); - XResIdAccList.insert(XResIdToAccObjHash::value_type(pObj.GetResID(),&(pIndTemp->second))); + { + std::scoped_lock l(m_Mutex); + + XIdAccList.insert( XIdToAccObjHash::value_type( pXAcc, pObj )); + XIdToAccObjHash::iterator pIndTemp = XIdAccList.find( pXAcc ); + XResIdAccList.insert(XResIdToAccObjHash::value_type(pObj.GetResID(),&(pIndTemp->second))); + } AccObject* pCurObj = GetAccObjByXAcc(pXAcc); if( pCurObj ) @@ -762,6 +793,8 @@ bool AccObjectWinManager::InsertAccObj( XAccessible* pXAcc,XAccessible* pParentX */ void AccObjectWinManager::SaveTopWindowHandle(HWND hWnd, css::accessibility::XAccessible* pXAcc) { + std::scoped_lock l(m_Mutex); + HwndXAcc.insert( XHWNDToXAccHash::value_type( hWnd,pXAcc ) ); } diff --git a/winaccessibility/source/service/msaaservice_impl.cxx b/winaccessibility/source/service/msaaservice_impl.cxx index 643a53f9662d..e66f2172fbbb 100644 --- a/winaccessibility/source/service/msaaservice_impl.cxx +++ b/winaccessibility/source/service/msaaservice_impl.cxx @@ -91,7 +91,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()) {
