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
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/152957
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>

7 files changed, 121 insertions, 81 deletions

diff --git a/winaccessibility/inc/AccEventListener.hxx b/winaccessibility/inc/AccEventListener.hxx
index 2d2875dfb060..f134981b20a7 100644
--- a/winaccessibility/inc/AccEventListener.hxx
+++ b/winaccessibility/inc/AccEventListener.hxx
@@ -73,7 +73,7 @@ public:
     //get the accessible parent's role
     virtual short GetParentRole();
 
-    void RemoveMeFromBroadcaster();
+    void RemoveMeFromBroadcaster(bool isNotifyDestroy);
 };
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/winaccessibility/inc/AccObject.hxx b/winaccessibility/inc/AccObject.hxx
index 3e259592e5be..53b1109b0d0e 100644
--- a/winaccessibility/inc/AccObject.hxx
+++ b/winaccessibility/inc/AccObject.hxx
@@ -50,7 +50,7 @@ private:
     short               m_accRole;
     long                m_resID;
     HWND                m_pParantID;
-    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;
@@ -87,7 +87,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 5c162dfed1ea..86a75c80fad7 100644
--- a/winaccessibility/inc/AccObjectWinManager.hxx
+++ b/winaccessibility/inc/AccObjectWinManager.hxx
@@ -20,6 +20,8 @@
 #pragma once
 
 #include <map>
+#include <mutex>
+
 #if !defined WIN32_LEAN_AND_MEAN
 # define WIN32_LEAN_AND_MEAN
 #endif
@@ -57,6 +59,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;
 
@@ -80,11 +85,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 b6b4e71f3aaa..a8f8e705fd2c 100644
--- a/winaccessibility/source/service/AccEventListener.cxx
+++ b/winaccessibility/source/service/AccEventListener.cxx
@@ -216,7 +216,7 @@ short AccEventListener::GetParentRole()
 /**
  *  remove the listener from accessible object
  */
-void AccEventListener::RemoveMeFromBroadcaster()
+void AccEventListener::RemoveMeFromBroadcaster(bool const isNotifyDestroy)
 {
     try
     {
@@ -237,7 +237,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 (...)
@@ -253,7 +256,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 d1e805ce2428..68dcc10105f2 100644
--- a/winaccessibility/source/service/AccObject.cxx
+++ b/winaccessibility/source/service/AccObject.cxx
@@ -162,7 +162,7 @@ AccObject::AccObject(XAccessible* pAcc, AccObjectManagerAgent* pAgent,
                      AccEventListener* pListener) :
         m_resID     (0),
         m_pParantID (nullptr),
-        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);
 }
 
@@ -1048,9 +1045,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 b0ee01525de9..c8e5c7ac936b 100644
--- a/winaccessibility/source/service/AccObjectWinManager.cxx
+++ b/winaccessibility/source/service/AccObjectWinManager.cxx
@@ -73,8 +73,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();
 }
@@ -94,13 +98,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 )
@@ -122,6 +120,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;
@@ -134,13 +134,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 = 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;
 }
 
 /**
@@ -391,6 +404,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())
@@ -433,35 +448,47 @@ 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();
+        if( accObj.GetIMAccessible() )
+        {
+            accObj.GetIMAccessible()->Release();
+        }
+        size_t i = XResIdAccList.erase(accObj.GetResID());
+        assert(i != 0);
+        (void) i;
+        DeleteFromHwndXAcc(pXAcc);
+        if (accObj.GetRole() == AccessibleRole::DOCUMENT ||
+            accObj.GetRole() == AccessibleRole::DOCUMENT_PRESENTATION ||
+            accObj.GetRole() == AccessibleRole::DOCUMENT_SPREADSHEET ||
+            accObj.GetRole() == AccessibleRole::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);
-    (void) i;
-    DeleteFromHwndXAcc(pXAcc);
-    if (accObj.GetRole() == AccessibleRole::DOCUMENT ||
-        accObj.GetRole() == AccessibleRole::DOCUMENT_PRESENTATION ||
-        accObj.GetRole() == AccessibleRole::DOCUMENT_SPREADSHEET ||
-        accObj.GetRole() == AccessibleRole::DOCUMENT_TEXT)
+    if (pListener)
     {
-        XHWNDDocList.erase(accObj.GetParentHWND());
+        pListener->RemoveMeFromBroadcaster(false);
     }
-    XIdAccList.erase(pXAcc); // note: this invalidates accObj so do it last!
 }
 
 /**
@@ -469,13 +496,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);
 }
 
 /**
@@ -568,29 +591,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)
@@ -600,6 +600,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)
@@ -647,9 +674,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 )
@@ -673,6 +704,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 ed9f88062720..dcc493718acc 100644
--- a/winaccessibility/source/service/msaaservice_impl.cxx
+++ b/winaccessibility/source/service/msaaservice_impl.cxx
@@ -85,7 +85,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())
     {