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 <assam.boudjelthia@qt.io>
(cherry picked from commit bf76387507dde71063ebe484bce1e7d1b8e09bc2)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 2b14b70b435fa1120dee3f08a1377aa26016d2b5)

src/corelib/platform/android/qandroiditemmodelproxy.cpp

@@ -11,9 +11,13 @@ QT_BEGIN_NAMESPACE
 
 using namespace QtJniTypes;
 
+std::map<const QAbstractItemModel *, QRecursiveMutex> QAndroidItemModelProxy::s_mutexes =
+        std::map<const QAbstractItemModel *, QRecursiveMutex>{};
+
 jint QAndroidItemModelProxy::columnCount(const QModelIndex &parent) const
 {
     Q_ASSERT(jInstance.isValid());
+    const QMutexLocker<QRecursiveMutex> lock = getMutexLocker(this);
     auto parentIndex = QAndroidModelIndexProxy::jInstance(parent);
     return jInstance.callMethod<jint>("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<QRecursiveMutex> lock = getMutexLocker(this);
     auto parentIndex = QAndroidModelIndexProxy::jInstance(parent);
     return jInstance.callMethod<jboolean>("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<QRecursiveMutex> lock = getMutexLocker(this);
     auto jIndex = QAndroidModelIndexProxy::jInstance(index);
     QJniObject jData = jInstance.callMethod<jobject>("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<QRecursiveMutex> lock = getMutexLocker(this);
     JQtModelIndex jIndex = jInstance.callMethod<JQtModelIndex>(
             "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<QRecursiveMutex> 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<QRecursiveMutex> lock = getMutexLocker(this);
 
     auto parentIndex = QAndroidModelIndexProxy::jInstance(parent);
     return jInstance.callMethod<int>("rowCount", parentIndex);
@@ -65,6 +74,7 @@ int QAndroidItemModelProxy::rowCount(const QModelIndex &parent) const
 QHash<int, QByteArray> QAndroidItemModelProxy::roleNames() const
 {
     Q_ASSERT(jInstance.isValid());
+    const QMutexLocker<QRecursiveMutex> lock = getMutexLocker(this);
 
     QHash<int, QByteArray> roleNames;
     HashMap hashMap = jInstance.callMethod<HashMap>("roleNames");
@@ -88,6 +98,7 @@ QHash<int, QByteArray> QAndroidItemModelProxy::defaultRoleNames() const
 void QAndroidItemModelProxy::fetchMore(const QModelIndex &parent)
 {
     Q_ASSERT(jInstance.isValid());
+    const QMutexLocker<QRecursiveMutex> lock = getMutexLocker(this);
     auto parentIndex = QAndroidModelIndexProxy::jInstance(parent);
     jInstance.callMethod<void>("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<QRecursiveMutex> lock = getMutexLocker(this);
     auto parentIndex = QAndroidModelIndexProxy::jInstance(parent);
     return jInstance.callMethod<jboolean>("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<QRecursiveMutex> lock = getMutexLocker(this);
     return QAndroidModelIndexProxy::qInstance(jInstance.callMethod<jobject>(
             "sibling", row, column, QAndroidModelIndexProxy::jInstance(parent)));
 }
@@ -143,8 +156,10 @@ QAndroidItemModelProxy::createNativeProxy(QJniObject itemModel)
         itemModel.callMethod<void>("setNativeReference", reinterpret_cast<jlong>(nativeProxy));
         connect(nativeProxy, &QAndroidItemModelProxy::destroyed, nativeProxy, [](QObject *obj) {
             auto proxy = qobject_cast<QAndroidItemModelProxy *>(obj);
-            if (proxy)
+            if (proxy) {
+                const QMutexLocker<QRecursiveMutex> lock = getMutexLocker(proxy);
                 proxy->jInstance.callMethod<void>("detachFromNative");
+            }
         });
     }
     return nativeProxy;

src/corelib/platform/android/qandroiditemmodelproxy_p.h

@@ -22,6 +22,7 @@
 #include <QtCore/qjnienvironment.h>
 #include <QtCore/qjnitypes.h>
 #include <QtCore/qthread.h>
+#include <QtCore/qmutex.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -65,7 +66,8 @@ public:
         Q_ASSERT(jvmObject);
         auto model = qobject_cast<QAndroidItemModelProxy *>(nativeInstance(jvmObject));
         Q_ASSERT(model);
-        return safeCall(model, std::forward<Func>(func), std::forward<Args>(args)...);
+        const QMutexLocker<QRecursiveMutex> lock = getMutexLocker(model);
+        return std::invoke(std::forward<Func>(func), model, std::forward<Args>(args)...);
     }
 
     template <typename Func, typename... Args>
@@ -74,7 +76,8 @@ public:
         Q_ASSERT(jvmObject);
         auto model = nativeInstance(jvmObject);
         Q_ASSERT(model);
-        return safeCall(model, std::forward<Func>(func), std::forward<Args>(args)...);
+        const QMutexLocker<QRecursiveMutex> lock = getMutexLocker(model);
+        return std::invoke(std::forward<Func>(func), model, std::forward<Args>(args)...);
     }
 
     template <typename Func1, typename Func2, typename... Args>
@@ -84,33 +87,13 @@ public:
         Q_ASSERT(jvmObject);
         auto nativeModel = nativeInstance(jvmObject);
         auto nativeProxyModel = qobject_cast<QAndroidItemModelProxy *>(nativeModel);
+        const QMutexLocker<QRecursiveMutex> lock = getMutexLocker(nativeModel);
         if (nativeProxyModel)
-            return safeCall(nativeProxyModel, std::forward<Func1>(defaultFunc),
+            return std::invoke(std::forward<Func1>(defaultFunc), nativeProxyModel,
-                            std::forward<Args>(args)...);
+                               std::forward<Args>(args)...);
         else
-            return safeCall(nativeModel, std::forward<Func2>(func), std::forward<Args>(args)...);
-    }
 
-    template <typename Object, typename Func, typename... Args>
+            return std::invoke(std::forward<Func2>(func), nativeModel, std::forward<Args>(args)...);
-    static auto safeCall(Object *object, Func &&func, Args &&...args)
-    {
-        using ReturnType = decltype(std::invoke(std::forward<Func>(func), object,
-                                                std::forward<Args>(args)...));
-
-        if constexpr (std::is_void_v<ReturnType>) {
-            QMetaObject::invokeMethod(object, std::forward<Func>(func), Qt::AutoConnection,
-                                      std::forward<Args>(args)...);
-        } else {
-            ReturnType returnValue;
-
-            const auto connectionType = object->thread() == QThread::currentThread()
-                    ? Qt::DirectConnection
-                    : Qt::BlockingQueuedConnection;
-
-            QMetaObject::invokeMethod(object, std::forward<Func>(func), connectionType,
-                                      qReturnArg(returnValue), std::forward<Args>(args)...);
-            return returnValue;
-        }
     }
 
     static jint jni_columnCount(JNIEnv *env, jobject object, JQtModelIndex parent);
@@ -206,6 +189,19 @@ public:
     static bool registerProxyNatives(QJniEnvironment &env);
 
 private:
+    static std::map<const QAbstractItemModel *, QRecursiveMutex> s_mutexes;
+
+    Q_REQUIRED_RESULT static QMutexLocker<QRecursiveMutex>
+    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<QAbstractItemModel *>(model));
+            });
+        return QMutexLocker(&iter->second);
+    }
+
     QJniObject jInstance;
     friend class QAndroidModelIndexProxy;
 };