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 <pavel.dubsky@qt.io>
Reviewed-by: Oliver Wolff <oliver.wolff@qt.io>
(cherry picked from commit caa560931929d544ab816de09ba5f23912585ade)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

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<QWindowsUiaMainProvider *>(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)

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

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)

src/plugins/platforms/windows/uiautomation/qwindowsuiaprovidercache.h

@@ -7,9 +7,10 @@
 #include <QtGui/qtguiglobal.h>
 #if QT_CONFIG(accessibility)
 
-#include "qwindowsuiabaseprovider.h"
+#include "qwindowsuiamainprovider.h"
 
 #include <QtCore/qhash.h>
+#include <QtCore/qmutex.h>
 #include <QtGui/qaccessible.h>
 
 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<QAccessible::Id, QWindowsUiaBaseProvider *> m_providerTable;
+    mutable QMutex m_tableMutex; // TODO: Can tables be accessed concurrently?
+    QHash<QAccessible::Id, QWindowsUiaMainProvider *> m_providerTable;
     QHash<QObject *, QAccessible::Id> m_inverseTable;
 };