diff --git a/src/corelib/kernel/qjniobject.cpp b/src/corelib/kernel/qjniobject.cpp index d96ad7e945..5a5239fc63 100644 --- a/src/corelib/kernel/qjniobject.cpp +++ b/src/corelib/kernel/qjniobject.cpp @@ -277,8 +277,19 @@ using namespace Qt::StringLiterals; class QJniObjectPrivate { public: - QJniObjectPrivate() = default; + // This is safe and necessary - the JNIEnv is attached to the current thread + // and the pointer remains valid for as long as the thread is alive. And if the + // QJniObject outlives the thread that it lives in, and gets called for + // anything other than destruction, then we have a data race anyway. + // And it's necessary, because the QJniEnvironment destructor calls + // checkAndClearExceptions, and since we have QJniObjects that get destroyed from + // a different thread than the one "owning" it, this triggers JNI assertions. + QJniObjectPrivate() + : m_env(QJniEnvironment().jniEnv()) + { + } ~QJniObjectPrivate() { + // use the environment of the current thread here QJniEnvironment env; if (m_jobject) env->DeleteGlobalRef(m_jobject); @@ -286,10 +297,16 @@ public: env->DeleteGlobalRef(m_jclass); } + JNIEnv *jniEnv() const noexcept + { + return m_env; + } + template - void construct(JNIEnv *env, const char *signature = nullptr, Args &&...args) + void construct(const char *signature = nullptr, Args &&...args) { if (m_jclass) { + JNIEnv *env = jniEnv(); // get default constructor jmethodID constructorId = QJniObject::getCachedMethodID(env, m_jclass, m_className, "", signature ? signature : "()V"); @@ -307,10 +324,11 @@ public: } } + QByteArray m_className; + JNIEnv * const m_env; jobject m_jobject = nullptr; jclass m_jclass = nullptr; bool m_own_jclass = true; - QByteArray m_className; }; template @@ -337,13 +355,12 @@ static jclass getCachedClass(const QByteArray &className) exception clearing and delete the local reference before returning. The JNI object can be null if there was an exception. */ -static QJniObject getCleanJniObject(jobject object) +static QJniObject getCleanJniObject(jobject object, JNIEnv *env) { if (!object) return QJniObject(); - QJniEnvironment env; - if (env.checkAndClearExceptions()) { + if (QJniEnvironment::checkAndClearExceptions(env)) { env->DeleteLocalRef(object); return QJniObject(); } @@ -392,7 +409,7 @@ jclass QtAndroidPrivate::findClass(const char *className, JNIEnv *env) } if (!clazz) { - // We didn't get an env. pointer or we got one with the WRONG class loader... + // Wrong class loader, try our own QJniObject classLoader(QtAndroidPrivate::classLoader()); if (!classLoader.isValid()) return nullptr; @@ -571,12 +588,11 @@ QJniObject::QJniObject() QJniObject::QJniObject(const char *className) : d(new QJniObjectPrivate()) { - QJniEnvironment env; d->m_className = className; - d->m_jclass = loadClass(d->m_className, env.jniEnv()); + d->m_jclass = loadClass(d->m_className, jniEnv()); d->m_own_jclass = false; - d->construct(env.jniEnv()); + d->construct(); } /*! @@ -595,14 +611,13 @@ QJniObject::QJniObject(const char *className) QJniObject::QJniObject(const char *className, const char *signature, ...) : d(new QJniObjectPrivate()) { - QJniEnvironment env; d->m_className = className; - d->m_jclass = loadClass(d->m_className, env.jniEnv()); + d->m_jclass = loadClass(d->m_className, jniEnv()); d->m_own_jclass = false; va_list args; va_start(args, signature); - d->construct(env.jniEnv(), signature, args); + d->construct(signature, args); va_end(args); } @@ -635,12 +650,11 @@ QJniObject::QJniObject(const char *className, const char *signature, ...) QJniObject::QJniObject(jclass clazz, const char *signature, ...) : d(new QJniObjectPrivate()) { - QJniEnvironment env; if (clazz) { - d->m_jclass = static_cast(env->NewGlobalRef(clazz)); + d->m_jclass = static_cast(jniEnv()->NewGlobalRef(clazz)); va_list args; va_start(args, signature); - d->construct(env.jniEnv(), signature, args); + d->construct(signature, args); va_end(args); } } @@ -691,7 +705,7 @@ QJniObject::QJniObject(jobject object) if (!object) return; - QJniEnvironment env; + JNIEnv *env = d->jniEnv(); d->m_jobject = env->NewGlobalRef(object); jclass cls = env->GetObjectClass(object); d->m_jclass = static_cast(env->NewGlobalRef(cls)); @@ -721,6 +735,13 @@ QJniObject::QJniObject(jobject object) QJniObject::~QJniObject() {} +/*! \internal +*/ +JNIEnv *QJniObject::jniEnv() const noexcept +{ + return d->jniEnv(); +} + /*! \fn template T QJniObject::object() const @@ -773,7 +794,7 @@ jclass QJniObject::objectClass() const QByteArray QJniObject::className() const { if (d->m_className.isEmpty() && d->m_jclass && d->m_jobject) { - QJniEnvironment env; + JNIEnv *env = jniEnv(); if (env->PushLocalFrame(3) != JNI_OK) // JVM out of memory return d->m_className; jmethodID mid = env->GetMethodID(d->m_jclass, "getClass", "()Ljava/lang/Class;"); @@ -934,12 +955,11 @@ QByteArray QJniObject::className() const */ QJniObject QJniObject::callObjectMethod(const char *methodName, const char *signature, ...) const { - QJniEnvironment env; - jmethodID id = getCachedMethodID(env.jniEnv(), methodName, signature); + jmethodID id = getCachedMethodID(jniEnv(), methodName, signature); if (id) { va_list args; va_start(args, signature); - QJniObject res = getCleanJniObject(env->CallObjectMethodV(d->m_jobject, id, args)); + QJniObject res = getCleanJniObject(jniEnv()->CallObjectMethodV(d->m_jobject, id, args), jniEnv()); va_end(args); return res; } @@ -972,7 +992,7 @@ QJniObject QJniObject::callStaticObjectMethod(const char *className, const char if (id) { va_list args; va_start(args, signature); - QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, id, args)); + QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, id, args), env.jniEnv()); va_end(args); return res; } @@ -990,13 +1010,13 @@ QJniObject QJniObject::callStaticObjectMethod(const char *className, const char QJniObject QJniObject::callStaticObjectMethod(jclass clazz, const char *methodName, const char *signature, ...) { - QJniEnvironment env; if (clazz) { + QJniEnvironment env; jmethodID id = getMethodID(env.jniEnv(), clazz, methodName, signature, true); if (id) { va_list args; va_start(args, signature); - QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, id, args)); + QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, id, args), env.jniEnv()); va_end(args); return res; } @@ -1022,11 +1042,11 @@ QJniObject QJniObject::callStaticObjectMethod(jclass clazz, const char *methodNa */ QJniObject QJniObject::callStaticObjectMethod(jclass clazz, jmethodID methodId, ...) { - QJniEnvironment env; if (clazz && methodId) { + QJniEnvironment env; va_list args; va_start(args, methodId); - QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, methodId, args)); + QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, methodId, args), env.jniEnv()); va_end(args); return res; } @@ -1171,7 +1191,7 @@ QJniObject QJniObject::getStaticObjectField(const char *className, if (!id) return QJniObject(); - return getCleanJniObject(env->GetStaticObjectField(clazz, id)); + return getCleanJniObject(env->GetStaticObjectField(clazz, id), env.jniEnv()); } /*! @@ -1194,7 +1214,7 @@ QJniObject QJniObject::getStaticObjectField(jclass clazz, const char *fieldName, if (!id) return QJniObject(); - return getCleanJniObject(env->GetStaticObjectField(clazz, id)); + return getCleanJniObject(env->GetStaticObjectField(clazz, id), env.jniEnv()); } /*! @@ -1233,12 +1253,11 @@ QJniObject QJniObject::getStaticObjectField(jclass clazz, const char *fieldName, */ QJniObject QJniObject::getObjectField(const char *fieldName, const char *signature) const { - QJniEnvironment env; - jfieldID id = getCachedFieldID(env.jniEnv(), fieldName, signature); + jfieldID id = getCachedFieldID(jniEnv(), fieldName, signature); if (!id) return QJniObject(); - return getCleanJniObject(env->GetObjectField(d->m_jobject, id)); + return getCleanJniObject(jniEnv()->GetObjectField(d->m_jobject, id), jniEnv()); } /*! @@ -1291,7 +1310,7 @@ QJniObject QJniObject::fromString(const QString &string) QJniEnvironment env; jstring stringRef = env->NewString(reinterpret_cast(string.constData()), string.length()); - QJniObject stringObject = getCleanJniObject(stringRef); + QJniObject stringObject = getCleanJniObject(stringRef, env.jniEnv()); stringObject.d->m_className = "java/lang/String"; return stringObject; } @@ -1316,10 +1335,9 @@ QString QJniObject::toString() const return QString(); QJniObject string = callObjectMethod("toString"); - QJniEnvironment env; - const int strLength = env->GetStringLength(string.object()); + const int strLength = string.jniEnv()->GetStringLength(string.object()); QString res(strLength, Qt::Uninitialized); - env->GetStringRegion(string.object(), 0, strLength, reinterpret_cast(res.data())); + string.jniEnv()->GetStringRegion(string.object(), 0, strLength, reinterpret_cast(res.data())); return res; } @@ -1377,13 +1395,17 @@ bool QJniObject::isValid() const QJniObject QJniObject::fromLocalRef(jobject lref) { QJniObject obj(lref); - QJniEnvironment()->DeleteLocalRef(lref); + obj.jniEnv()->DeleteLocalRef(lref); return obj; } bool QJniObject::isSameObject(jobject obj) const { - return QJniEnvironment()->IsSameObject(d->m_jobject, obj); + if (d->m_jobject == obj) + return true; + if (!d->m_jobject || !obj) + return false; + return jniEnv()->IsSameObject(d->m_jobject, obj); } bool QJniObject::isSameObject(const QJniObject &other) const @@ -1398,7 +1420,7 @@ void QJniObject::assign(jobject obj) d = QSharedPointer::create(); if (obj) { - QJniEnvironment env; + JNIEnv *env = d->jniEnv(); d->m_jobject = env->NewGlobalRef(obj); jclass objectClass = env->GetObjectClass(obj); d->m_jclass = static_cast(env->NewGlobalRef(objectClass)); diff --git a/src/corelib/kernel/qjniobject.h b/src/corelib/kernel/qjniobject.h index 88214a9311..1f6433d44e 100644 --- a/src/corelib/kernel/qjniobject.h +++ b/src/corelib/kernel/qjniobject.h @@ -18,24 +18,42 @@ class Q_CORE_EXPORT QJniObject { template struct LocalFrame { - QJniEnvironment env; + mutable JNIEnv *env; bool hasFrame = false; - ~LocalFrame() { + explicit LocalFrame(JNIEnv *env = nullptr) noexcept + : env(env) + { + } + ~LocalFrame() + { if (hasFrame) env->PopLocalFrame(nullptr); } template - auto newLocalRef(QJniObject &&object) { + auto newLocalRef(jobject object) + { if (!hasFrame) { - if (env->PushLocalFrame(sizeof...(Args)) < 0) + if (jniEnv()->PushLocalFrame(sizeof...(Args)) < 0) return T{}; // JVM is out of memory, avoid making matters worse hasFrame = true; } - return static_cast(env->NewLocalRef(object.template object())); + return static_cast(jniEnv()->NewLocalRef(object)); + } + template + auto newLocalRef(QJniObject &&object) + { + return newLocalRef(object.template object()); + } + JNIEnv *jniEnv() const + { + if (!env) + env = QJniEnvironment().jniEnv(); + return env; + } + bool checkAndClearExceptions() + { + return env ? QJniEnvironment::checkAndClearExceptions(env) : false; } - JNIEnv *jniEnv() const { return env.jniEnv(); } - bool checkAndClearExceptions() { return env.checkAndClearExceptions(); } - template auto convertToJni(T &&value); template @@ -103,7 +121,7 @@ public: > auto callMethod(const char *methodName, const char *signature, Args &&...args) const { - LocalFrame frame; + LocalFrame frame(jniEnv()); if constexpr (QtJniTypes::isObjectType()) { return frame.template convertFromJni(callObjectMethod(methodName, signature, frame.convertToJni(std::forward(args))...)); @@ -148,7 +166,7 @@ public: { QtJniTypes::assertObjectType(); constexpr auto signature = QtJniTypes::methodSignature(); - LocalFrame frame; + LocalFrame frame(jniEnv()); return frame.template convertFromJni(callObjectMethod(methodName, signature, frame.convertToJni(std::forward(args))...)); } @@ -212,7 +230,10 @@ public: { QJniEnvironment env; jclass clazz = QJniObject::loadClass(className, env.jniEnv()); - return callStaticMethod(clazz, methodName, std::forward(args)...); + const jmethodID id = clazz ? getMethodID(env.jniEnv(), clazz, methodName, + QtJniTypes::methodSignature().data(), true) + : 0; + return callStaticMethod(clazz, id, std::forward(args)...); } template auto getField(const char *fieldName) const { - LocalFrame frame; + LocalFrame frame(jniEnv()); if constexpr (QtJniTypes::isObjectType()) { return frame.template convertFromJni(getObjectField(fieldName)); } else { @@ -401,12 +422,11 @@ public: > void setField(const char *fieldName, T value) { - QJniEnvironment env; constexpr auto signature = QtJniTypes::fieldSignature(); - jfieldID id = getCachedFieldID(env.jniEnv(), fieldName, signature); + jfieldID id = getCachedFieldID(jniEnv(), fieldName, signature); if (id) { - setFieldForType(env.jniEnv(), object(), id, value); - env.checkAndClearExceptions(); + setFieldForType(jniEnv(), object(), id, value); + QJniEnvironment::checkAndClearExceptions(jniEnv()); } } @@ -417,11 +437,10 @@ public: > void setField(const char *fieldName, const char *signature, T value) { - QJniEnvironment env; - jfieldID id = getCachedFieldID(env.jniEnv(), fieldName, signature); + jfieldID id = getCachedFieldID(jniEnv(), fieldName, signature); if (id) { - setFieldForType(env.jniEnv(), object(), id, value); - env.checkAndClearExceptions(); + setFieldForType(jniEnv(), object(), id, value); + QJniEnvironment::checkAndClearExceptions(jniEnv()); } } @@ -524,6 +543,7 @@ public: protected: QJniObject(Qt::Initialization) {} + JNIEnv *jniEnv() const noexcept; private: static jclass loadClass(const QByteArray &className, JNIEnv *env); @@ -686,7 +706,7 @@ private: static constexpr void setFieldForType(JNIEnv *env, jobject obj, jfieldID id, T value) { - LocalFrame frame; + LocalFrame frame(env); if constexpr (sameTypeForJni) env->SetBooleanField(obj, id, static_cast(value)); else if constexpr (sameTypeForJni) @@ -713,7 +733,7 @@ private: static constexpr void setStaticFieldForType(JNIEnv *env, jclass clazz, jfieldID id, T value) { - LocalFrame frame; + LocalFrame frame(env); if constexpr (sameTypeForJni) env->SetStaticBooleanField(clazz, id, static_cast(value)); else if constexpr (sameTypeForJni) diff --git a/tests/auto/corelib/kernel/qjniobject/tst_qjniobject.cpp b/tests/auto/corelib/kernel/qjniobject/tst_qjniobject.cpp index 364b0c5d62..c39a6d0343 100644 --- a/tests/auto/corelib/kernel/qjniobject/tst_qjniobject.cpp +++ b/tests/auto/corelib/kernel/qjniobject/tst_qjniobject.cpp @@ -307,7 +307,7 @@ void tst_QJniObject::compareOperatorTests() QJniObject stringObject2 = QJniObject::fromString(str); QVERIFY(stringObject != stringObject2); - jstring jstrobj = 0; + jstring jstrobj = nullptr; QJniObject invalidStringObject; QVERIFY(invalidStringObject == jstrobj);