From 29c32e7cdc6463671dc0e31029ac39371a1728c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8ger=20Hanseg=C3=A5rd?= Date: Sun, 30 Jun 2024 17:12:17 +0200 Subject: [PATCH] Avoid holding static mutex in QWindowsUiaMainProvider::Release This patch reworks "Fix cache maybe invalid while the signal is actived from queue" (80f44954f6872afb5aa37e6737c3e1ac68ad3577) and associates a mutex with the QWindowsUiaProviderCache tables instead of with the QWindowsUiaMainProvider reference counting. This makes it easier to switch to using COM smart pointers to manage QWindowsUiaMainProvider lifetime, where AddRef/Release may be called as part of copying smart pointer instances. It is unclear if the QWindowsUiaProviderCache tables can be concurrently accessed, still this patch maintains a mutex to protect them. Task-number: QTBUG-106653 Task-number: QTBUG-126530 Change-Id: I2fc3ba6b6666412ebc86461ebc8f008e1fd6a165 Reviewed-by: Pavel Dubsky Reviewed-by: Oliver Wolff (cherry picked from commit caa560931929d544ab816de09ba5f23912585ade) Reviewed-by: Qt Cherry-pick Bot --- .../uiautomation/qwindowsuiamainprovider.cpp | 17 ++--------- .../uiautomation/qwindowsuiamainprovider.h | 2 -- .../uiautomation/qwindowsuiaprovidercache.cpp | 30 +++++++++++-------- .../uiautomation/qwindowsuiaprovidercache.h | 13 ++++---- 4 files changed, 26 insertions(+), 36 deletions(-) diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.cpp b/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.cpp index b4ac139b0f0..051af8b629f 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.cpp +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.cpp @@ -39,23 +39,17 @@ QT_BEGIN_NAMESPACE using namespace QWindowsUiAutomation; -QMutex QWindowsUiaMainProvider::m_mutex; - // Returns a cached instance of the provider for a specific accessible interface. QWindowsUiaMainProvider *QWindowsUiaMainProvider::providerForAccessible(QAccessibleInterface *accessible) { - QMutexLocker locker(&m_mutex); - if (!accessible) return nullptr; QAccessible::Id id = QAccessible::uniqueId(accessible); QWindowsUiaProviderCache *providerCache = QWindowsUiaProviderCache::instance(); - auto *provider = qobject_cast(providerCache->providerForId(id)); + QWindowsUiaMainProvider *provider = providerCache->providerForId(id); - if (provider) { - provider->AddRef(); - } else { + if (!provider) { provider = new QWindowsUiaMainProvider(accessible); providerCache->insert(id, provider); } @@ -257,13 +251,6 @@ HRESULT STDMETHODCALLTYPE QWindowsUiaMainProvider::QueryInterface(REFIID iid, LP return result; } -ULONG STDMETHODCALLTYPE QWindowsUiaMainProvider::Release() -{ - QMutexLocker locker(&m_mutex); - - return QComObject::Release(); -} - HRESULT QWindowsUiaMainProvider::get_ProviderOptions(ProviderOptions *pRetVal) { if (!pRetVal) diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.h b/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.h index 8ea343e4253..f9cf22a2dab 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.h +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.h @@ -38,7 +38,6 @@ public: // IUnknown HRESULT STDMETHODCALLTYPE QueryInterface(REFIID id, LPVOID *iface) override; - ULONG STDMETHODCALLTYPE Release() override; // IRawElementProviderSimple methods HRESULT STDMETHODCALLTYPE get_ProviderOptions(ProviderOptions *pRetVal) override; @@ -64,7 +63,6 @@ private: static void setStyle(QAccessibleInterface *accessible, VARIANT *pRetVal); /** Returns the UIA style ID for a heading level from 1 to 9. */ static int styleIdForHeadingLevel(int headingLevel); - static QMutex m_mutex; }; QT_END_NAMESPACE diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.cpp b/src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.cpp index ba2a88bb4ef..9f8f5a93ebf 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.cpp +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.cpp @@ -26,29 +26,40 @@ QWindowsUiaProviderCache *QWindowsUiaProviderCache::instance() } // Returns the provider instance associated with the ID, or nullptr. -QWindowsUiaBaseProvider *QWindowsUiaProviderCache::providerForId(QAccessible::Id id) const +QWindowsUiaMainProvider *QWindowsUiaProviderCache::providerForId(QAccessible::Id id) const { - return m_providerTable.value(id); + QMutexLocker guard{ &m_tableMutex }; + QWindowsUiaMainProvider *provider = m_providerTable.value(id); + if (provider) + provider->AddRef(); // Make sure lifetime is extended while holding the mutex + return provider; } // Inserts a provider in the cache and associates it with an accessibility ID. -void QWindowsUiaProviderCache::insert(QAccessible::Id id, QWindowsUiaBaseProvider *provider) +void QWindowsUiaProviderCache::insert(QAccessible::Id id, QWindowsUiaMainProvider *provider) { - remove(id); + QMutexLocker guard{ &m_tableMutex }; + // Remove id if it already exists + m_inverseTable.remove(m_providerTable.value(id)); + m_providerTable.remove(id); + + // Add new provider if (provider) { m_providerTable[id] = provider; m_inverseTable[provider] = id; + guard.unlock(); // Connects the destroyed signal to our slot, to remove deleted objects from the cache. - QObject::connect(provider, &QObject::destroyed, this, &QWindowsUiaProviderCache::objectDestroyed, Qt::DirectConnection); + QObject::connect(provider, &QObject::destroyed, this, &QWindowsUiaProviderCache::remove, Qt::DirectConnection); } } // Removes deleted provider objects from the cache. -void QWindowsUiaProviderCache::objectDestroyed(QObject *obj) +void QWindowsUiaProviderCache::remove(QObject *obj) { // We have to use the inverse table to map the object address back to its ID, // since at this point (called from QObject destructor), it has already been // partially destroyed and we cannot treat it as a provider. + QMutexLocker guard{ &m_tableMutex }; auto it = m_inverseTable.find(obj); if (it != m_inverseTable.end()) { m_providerTable.remove(*it); @@ -56,13 +67,6 @@ void QWindowsUiaProviderCache::objectDestroyed(QObject *obj) } } -// Removes a provider with a given id from the cache. -void QWindowsUiaProviderCache::remove(QAccessible::Id id) -{ - m_inverseTable.remove(m_providerTable.value(id)); - m_providerTable.remove(id); -} - QT_END_NAMESPACE #endif // QT_CONFIG(accessibility) diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.h b/src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.h index 08f315b4e69..0351587f22b 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.h +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.h @@ -7,9 +7,10 @@ #include #if QT_CONFIG(accessibility) -#include "qwindowsuiabaseprovider.h" +#include "qwindowsuiamainprovider.h" #include +#include #include QT_BEGIN_NAMESPACE @@ -21,15 +22,15 @@ class QWindowsUiaProviderCache : public QObject Q_OBJECT public: static QWindowsUiaProviderCache *instance(); - QWindowsUiaBaseProvider *providerForId(QAccessible::Id id) const; - void insert(QAccessible::Id id, QWindowsUiaBaseProvider *provider); - void remove(QAccessible::Id id); + QWindowsUiaMainProvider *providerForId(QAccessible::Id id) const; + void insert(QAccessible::Id id, QWindowsUiaMainProvider *provider); private Q_SLOTS: - void objectDestroyed(QObject *obj); + void remove(QObject *obj); private: - QHash m_providerTable; + mutable QMutex m_tableMutex; // TODO: Can tables be accessed concurrently? + QHash m_providerTable; QHash m_inverseTable; };