From e7fde4958d0885ed6bd569d0ec1f520f351278ce Mon Sep 17 00:00:00 2001 From: Soheil Armin Date: Fri, 1 Nov 2024 11:59:07 +0200 Subject: [PATCH] Android: Fix synchronization issue of QtAbstractItemModel and its proxy Java methods of QtAbstractItemModel may get called from Qt GUI thread or Java main thread. Situation is the same with internal proxy methods, where they may directly or indirectly called from these two thread. At the same time, a chain of calls may happen from these threads, so some functions may get called indirectly from a single caller thread. In this change, we are adding a map of QObject* to QRecursiveMutux where the QObject is a QAbstractItemModel, either the proxy instance or a reference to an underlaying QAbstractItemModel when the model is implemented in C++. Each call to any function locks the QRecursiveMutux instance associated with the QAbstractItemModel instance. A connection from QObject::destroyed to a lamda, helps to remove the map entry when the model is destroyed. Fixes: QTBUG-127467 Change-Id: I18dcdc207e201b3883526648b4e672e5868ed8ab Reviewed-by: Assam Boudjelthia (cherry picked from commit bf76387507dde71063ebe484bce1e7d1b8e09bc2) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 2b14b70b435fa1120dee3f08a1377aa26016d2b5) --- .../android/qandroiditemmodelproxy.cpp | 17 ++++++- .../android/qandroiditemmodelproxy_p.h | 48 +++++++++---------- 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/src/corelib/platform/android/qandroiditemmodelproxy.cpp b/src/corelib/platform/android/qandroiditemmodelproxy.cpp index e71c5a52d88..e5e5b6f2f3c 100644 --- a/src/corelib/platform/android/qandroiditemmodelproxy.cpp +++ b/src/corelib/platform/android/qandroiditemmodelproxy.cpp @@ -11,9 +11,13 @@ QT_BEGIN_NAMESPACE using namespace QtJniTypes; +std::map QAndroidItemModelProxy::s_mutexes = + std::map{}; + jint QAndroidItemModelProxy::columnCount(const QModelIndex &parent) const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); auto parentIndex = QAndroidModelIndexProxy::jInstance(parent); return jInstance.callMethod("columnCount", parentIndex); } @@ -21,6 +25,7 @@ jint QAndroidItemModelProxy::columnCount(const QModelIndex &parent) const bool QAndroidItemModelProxy::canFetchMore(const QModelIndex &parent) const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); auto parentIndex = QAndroidModelIndexProxy::jInstance(parent); return jInstance.callMethod("canFetchMore", parentIndex); } @@ -33,6 +38,7 @@ bool QAndroidItemModelProxy::canFetchMoreDefault(const QModelIndex &parent) cons QVariant QAndroidItemModelProxy::data(const QModelIndex &index, int role) const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); auto jIndex = QAndroidModelIndexProxy::jInstance(index); QJniObject jData = jInstance.callMethod("data", jIndex, role); return QAndroidTypeConverter::toQVariant(jData); @@ -41,6 +47,7 @@ QVariant QAndroidItemModelProxy::data(const QModelIndex &index, int role) const QModelIndex QAndroidItemModelProxy::index(int row, int column, const QModelIndex &parent) const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); JQtModelIndex jIndex = jInstance.callMethod( "index", row, column, QAndroidModelIndexProxy::jInstance(parent)); return QAndroidModelIndexProxy::qInstance(jIndex); @@ -49,6 +56,7 @@ QModelIndex QAndroidItemModelProxy::index(int row, int column, const QModelIndex QModelIndex QAndroidItemModelProxy::parent(const QModelIndex &index) const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); auto jIndex = QAndroidModelIndexProxy::jInstance(index); return QAndroidModelIndexProxy::qInstance( @@ -57,6 +65,7 @@ QModelIndex QAndroidItemModelProxy::parent(const QModelIndex &index) const int QAndroidItemModelProxy::rowCount(const QModelIndex &parent) const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); auto parentIndex = QAndroidModelIndexProxy::jInstance(parent); return jInstance.callMethod("rowCount", parentIndex); @@ -65,6 +74,7 @@ int QAndroidItemModelProxy::rowCount(const QModelIndex &parent) const QHash QAndroidItemModelProxy::roleNames() const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); QHash roleNames; HashMap hashMap = jInstance.callMethod("roleNames"); @@ -88,6 +98,7 @@ QHash QAndroidItemModelProxy::defaultRoleNames() const void QAndroidItemModelProxy::fetchMore(const QModelIndex &parent) { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); auto parentIndex = QAndroidModelIndexProxy::jInstance(parent); jInstance.callMethod("fetchMore", parentIndex); } @@ -100,6 +111,7 @@ void QAndroidItemModelProxy::fetchMoreDefault(const QModelIndex &parent) bool QAndroidItemModelProxy::hasChildren(const QModelIndex &parent) const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); auto parentIndex = QAndroidModelIndexProxy::jInstance(parent); return jInstance.callMethod("hasChildren", parentIndex); } @@ -112,6 +124,7 @@ bool QAndroidItemModelProxy::hasChildrenDefault(const QModelIndex &parent) const QModelIndex QAndroidItemModelProxy::sibling(int row, int column, const QModelIndex &parent) const { Q_ASSERT(jInstance.isValid()); + const QMutexLocker lock = getMutexLocker(this); return QAndroidModelIndexProxy::qInstance(jInstance.callMethod( "sibling", row, column, QAndroidModelIndexProxy::jInstance(parent))); } @@ -143,8 +156,10 @@ QAndroidItemModelProxy::createNativeProxy(QJniObject itemModel) itemModel.callMethod("setNativeReference", reinterpret_cast(nativeProxy)); connect(nativeProxy, &QAndroidItemModelProxy::destroyed, nativeProxy, [](QObject *obj) { auto proxy = qobject_cast(obj); - if (proxy) + if (proxy) { + const QMutexLocker lock = getMutexLocker(proxy); proxy->jInstance.callMethod("detachFromNative"); + } }); } return nativeProxy; diff --git a/src/corelib/platform/android/qandroiditemmodelproxy_p.h b/src/corelib/platform/android/qandroiditemmodelproxy_p.h index 9f5369af47a..3843faf9cdc 100644 --- a/src/corelib/platform/android/qandroiditemmodelproxy_p.h +++ b/src/corelib/platform/android/qandroiditemmodelproxy_p.h @@ -22,6 +22,7 @@ #include #include #include +#include QT_BEGIN_NAMESPACE @@ -65,7 +66,8 @@ public: Q_ASSERT(jvmObject); auto model = qobject_cast(nativeInstance(jvmObject)); Q_ASSERT(model); - return safeCall(model, std::forward(func), std::forward(args)...); + const QMutexLocker lock = getMutexLocker(model); + return std::invoke(std::forward(func), model, std::forward(args)...); } template @@ -74,7 +76,8 @@ public: Q_ASSERT(jvmObject); auto model = nativeInstance(jvmObject); Q_ASSERT(model); - return safeCall(model, std::forward(func), std::forward(args)...); + const QMutexLocker lock = getMutexLocker(model); + return std::invoke(std::forward(func), model, std::forward(args)...); } template @@ -84,33 +87,13 @@ public: Q_ASSERT(jvmObject); auto nativeModel = nativeInstance(jvmObject); auto nativeProxyModel = qobject_cast(nativeModel); + const QMutexLocker lock = getMutexLocker(nativeModel); if (nativeProxyModel) - return safeCall(nativeProxyModel, std::forward(defaultFunc), - std::forward(args)...); + return std::invoke(std::forward(defaultFunc), nativeProxyModel, + std::forward(args)...); else - return safeCall(nativeModel, std::forward(func), std::forward(args)...); - } - template - static auto safeCall(Object *object, Func &&func, Args &&...args) - { - using ReturnType = decltype(std::invoke(std::forward(func), object, - std::forward(args)...)); - - if constexpr (std::is_void_v) { - QMetaObject::invokeMethod(object, std::forward(func), Qt::AutoConnection, - std::forward(args)...); - } else { - ReturnType returnValue; - - const auto connectionType = object->thread() == QThread::currentThread() - ? Qt::DirectConnection - : Qt::BlockingQueuedConnection; - - QMetaObject::invokeMethod(object, std::forward(func), connectionType, - qReturnArg(returnValue), std::forward(args)...); - return returnValue; - } + return std::invoke(std::forward(func), nativeModel, std::forward(args)...); } static jint jni_columnCount(JNIEnv *env, jobject object, JQtModelIndex parent); @@ -206,6 +189,19 @@ public: static bool registerProxyNatives(QJniEnvironment &env); private: + static std::map s_mutexes; + + Q_REQUIRED_RESULT static QMutexLocker + getMutexLocker(const QAbstractItemModel *model) + { + auto [iter, inserted] = s_mutexes.try_emplace(model); + if (inserted) + QObject::connect(model, &QAbstractItemModel::destroyed, model, [](QObject *model) { + s_mutexes.erase(reinterpret_cast(model)); + }); + return QMutexLocker(&iter->second); + } + QJniObject jInstance; friend class QAndroidModelIndexProxy; };