Skip to content

Commit

Permalink
attempt to fix crashers when initializing the JVM
Browse files Browse the repository at this point in the history 
- clarify precondition for VirtualMachineEnv, in particular, 
  JNI_OnLoad() must be called from a thread attached to the VM, and
  should be called once. This is now enforced internally, subsequent
  calls are ignored.

- sVirtualMachine was written and read from potentially different 
  threads without synchronization. It is now mutex protected.

- added fatal asserts in all locations that we cannot recover from;
  previously we would stay in an undefined state.

BUGS=[390724273]
  • Loading branch information
@pixelflinger
pixelflinger committed Jan 31, 2025
1 parent 87bdf96 commit 42a5757
Show file tree
Hide file tree
Showing 4 changed files with 136 additions and 79 deletions.
42 changes: 14 additions & 28 deletions filament/backend/include/private/backend/VirtualMachineEnv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,33 +18,25 @@
#define TNT_FILAMENT_DRIVER_ANDROID_VIRTUAL_MACHINE_ENV_H

#include <utils/compiler.h>
#include <utils/debug.h>
#include <utils/Mutex.h>

#include <jni.h>

namespace filament {

class VirtualMachineEnv {
public:
// must be called before VirtualMachineEnv::get() from a thread that is attached to the JavaVM
static jint JNI_OnLoad(JavaVM* vm) noexcept;

static VirtualMachineEnv& get() noexcept {
// declaring this thread local, will ensure it's destroyed with the calling thread
static thread_local VirtualMachineEnv instance;
return instance;
}
// must be called on backend thread
static VirtualMachineEnv& get() noexcept;

static JNIEnv* getThreadEnvironment() noexcept {
JNIEnv* env;
assert_invariant(sVirtualMachine);
if (sVirtualMachine->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK) {
return nullptr; // this should not happen
}
return env;
}
// can be called from any thread that already has a JniEnv
static JNIEnv* getThreadEnvironment() noexcept;

inline JNIEnv* getEnvironment() noexcept {
assert_invariant(mVirtualMachine);
// must be called from the backend thread
JNIEnv* getEnvironment() noexcept {
JNIEnv* env = mJniEnv;
if (UTILS_UNLIKELY(!env)) {
return getEnvironmentSlow();
Expand All @@ -55,20 +47,14 @@ class VirtualMachineEnv {
static void handleException(JNIEnv* env) noexcept;

private:
VirtualMachineEnv() noexcept : mVirtualMachine(sVirtualMachine) {
// We're not initializing the JVM here -- but we could -- because most of the time
// we don't need the jvm. Instead we do the initialization on first use. This means we could get
// a nasty slow down the very first time, but we'll live with it for now.
}

~VirtualMachineEnv() {
if (mVirtualMachine) {
mVirtualMachine->DetachCurrentThread();
}
}

explicit VirtualMachineEnv(JavaVM* vm) noexcept;
~VirtualMachineEnv() noexcept;
JNIEnv* getEnvironmentSlow() noexcept;

static utils::Mutex sLock;
static JavaVM* sVirtualMachine;
static JavaVM* getVirtualMachine();

JNIEnv* mJniEnv = nullptr;
JavaVM* mVirtualMachine = nullptr;
};
Expand Down
89 changes: 77 additions & 12 deletions filament/backend/src/VirtualMachineEnv.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,28 @@

#include <utils/compiler.h>
#include <utils/debug.h>
#include <utils/Mutex.h>
#include <utils/Panic.h>

#include <jni.h>

#include <mutex>

namespace filament {

JavaVM* VirtualMachineEnv::sVirtualMachine = nullptr;
using namespace utils;

// This Mutex shouldn't be subject to the Static Initialization Order Fiasco because its initial state is
// a single int initialized to 0.
/* static*/ Mutex VirtualMachineEnv::sLock;
/* static*/ JavaVM* VirtualMachineEnv::sVirtualMachine = nullptr;

UTILS_NOINLINE
JavaVM* VirtualMachineEnv::getVirtualMachine() {
std::lock_guard const lock(sLock);
assert_invariant(sVirtualMachine);
return sVirtualMachine;
}

/*
* This is typically called by filament_jni.so when it is loaded. If filament_jni.so is not used,
Expand All @@ -35,33 +51,82 @@ JavaVM* VirtualMachineEnv::sVirtualMachine = nullptr;
UTILS_PUBLIC
UTILS_NOINLINE
jint VirtualMachineEnv::JNI_OnLoad(JavaVM* vm) noexcept {
JNIEnv* env = nullptr;
if (UTILS_UNLIKELY(vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK)) {
// this should not happen
return -1;
std::lock_guard const lock(sLock);
if (sVirtualMachine) {
// It doesn't make sense for JNI_OnLoad() to be called more than once
return JNI_VERSION_1_6;
}

// Here we check this VM at least has JNI_VERSION_1_6
JNIEnv* env = nullptr;
jint const result = vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6);

FILAMENT_CHECK_POSTCONDITION(result == JNI_OK)
<< "Couldn't get JniEnv* from the VM, error = " << result;

sVirtualMachine = vm;
return JNI_VERSION_1_6;
}

UTILS_NOINLINE
void VirtualMachineEnv::handleException(JNIEnv* const env) noexcept {
if (UTILS_UNLIKELY(env->ExceptionCheck())) {
env->ExceptionDescribe();
env->ExceptionClear();
VirtualMachineEnv& VirtualMachineEnv::get() noexcept {
JavaVM* const vm = getVirtualMachine();
// declaring this thread local, will ensure it's destroyed with the calling thread
thread_local VirtualMachineEnv instance{ vm };
return instance;
}

UTILS_NOINLINE
JNIEnv* VirtualMachineEnv::getThreadEnvironment() noexcept {
JavaVM* const vm = getVirtualMachine();
JNIEnv* env = nullptr;
jint const result = vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6);

FILAMENT_CHECK_POSTCONDITION(result == JNI_OK)
<< "Couldn't get JniEnv* from the VM, error = " << result;

return env;
}

VirtualMachineEnv::VirtualMachineEnv(JavaVM* vm) noexcept : mVirtualMachine(vm) {
// We're not initializing the JVM here -- but we could -- because most of the time
// we don't need the jvm. Instead, we do the initialization on first use. This means we could get
// a nasty slow down the very first time, but we'll live with it for now.
}

VirtualMachineEnv::~VirtualMachineEnv() noexcept {
if (mVirtualMachine) {
mVirtualMachine->DetachCurrentThread();
}
}

UTILS_NOINLINE
JNIEnv* VirtualMachineEnv::getEnvironmentSlow() noexcept {
FILAMENT_CHECK_PRECONDITION(mVirtualMachine)
<< "JNI_OnLoad() has not been called";

#if defined(__ANDROID__)
mVirtualMachine->AttachCurrentThread(&mJniEnv, nullptr);
jint const result = mVirtualMachine->AttachCurrentThread(&mJniEnv, nullptr);
#else
mVirtualMachine->AttachCurrentThread(reinterpret_cast<void**>(&mJniEnv), nullptr);
jint const result = mVirtualMachine->AttachCurrentThread(reinterpret_cast<void**>(&mJniEnv), nullptr);
#endif
assert_invariant(mJniEnv);

FILAMENT_CHECK_POSTCONDITION(result == JNI_OK)
<< "JavaVM::AttachCurrentThread failed with error " << result;

FILAMENT_CHECK_POSTCONDITION(mJniEnv)
<< "JavaVM::AttachCurrentThread returned a null mJniEnv";

return mJniEnv;
}

UTILS_NOINLINE
void VirtualMachineEnv::handleException(JNIEnv* const env) noexcept {
if (UTILS_UNLIKELY(env->ExceptionCheck())) {
env->ExceptionDescribe();
env->ExceptionClear();
}
}

} // namespace filament

73 changes: 37 additions & 36 deletions filament/backend/src/opengl/platforms/ExternalStreamManagerAndroid.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,28 @@

#include "ExternalStreamManagerAndroid.h"

#include <utils/api_level.h>
#include <private/backend/VirtualMachineEnv.h>

#include <backend/Platform.h>

#include <utils/compiler.h>
#include <utils/debug.h>
#include <utils/Log.h>
#include <utils/ostream.h>

#if __has_include(<android/surface_texture.h>)
# include <android/surface_texture.h>
# include <android/surface_texture_jni.h>
#else
struct ASurfaceTexture;
typedef struct ASurfaceTexture ASurfaceTexture;
#endif

#include <dlfcn.h>
#include <jni.h>

#include <chrono>
#include <stdint.h>

#include <new>

using namespace utils;

Expand All @@ -32,13 +47,8 @@ using namespace backend;
using Stream = Platform::Stream;


template <typename T>
static void loadSymbol(T*& pfn, const char *symbol) noexcept {
pfn = (T*)dlsym(RTLD_DEFAULT, symbol);
}

ExternalStreamManagerAndroid& ExternalStreamManagerAndroid::create() noexcept {
return *(new ExternalStreamManagerAndroid{});
return *(new(std::nothrow) ExternalStreamManagerAndroid{});
}

void ExternalStreamManagerAndroid::destroy(ExternalStreamManagerAndroid* pExternalStreamManagerAndroid) noexcept {
Expand All @@ -56,23 +66,13 @@ ExternalStreamManagerAndroid::~ExternalStreamManagerAndroid() noexcept = default

UTILS_NOINLINE
JNIEnv* ExternalStreamManagerAndroid::getEnvironmentSlow() noexcept {
JNIEnv * env = mVm.getEnvironment();
JNIEnv* const env = mVm.getEnvironment();
mJniEnv = env;

jclass SurfaceTextureClass = env->FindClass("android/graphics/SurfaceTexture");

mSurfaceTextureClass_updateTexImage = env->GetMethodID(
SurfaceTextureClass, "updateTexImage", "()V");

mSurfaceTextureClass_attachToGLContext = env->GetMethodID(
SurfaceTextureClass, "attachToGLContext", "(I)V");

mSurfaceTextureClass_detachFromGLContext = env->GetMethodID(
SurfaceTextureClass, "detachFromGLContext", "()V");

mSurfaceTextureClass_getTimestamp = env->GetMethodID(
SurfaceTextureClass, "getTimestamp", "()J");

mSurfaceTextureClass_updateTexImage = env->GetMethodID(SurfaceTextureClass, "updateTexImage", "()V");
mSurfaceTextureClass_attachToGLContext = env->GetMethodID(SurfaceTextureClass, "attachToGLContext", "(I)V");
mSurfaceTextureClass_detachFromGLContext = env->GetMethodID(SurfaceTextureClass, "detachFromGLContext", "()V");
mSurfaceTextureClass_getTimestamp = env->GetMethodID(SurfaceTextureClass, "getTimestamp", "()J");
return env;
}

Expand All @@ -82,7 +82,7 @@ Stream* ExternalStreamManagerAndroid::acquire(jobject surfaceTexture) noexcept {
if (!env) {
return nullptr; // this should not happen
}
EGLStream* stream = new EGLStream();
EGLStream* stream = new(std::nothrow) EGLStream();
stream->jSurfaceTexture = env->NewGlobalRef(surfaceTexture);
if (__builtin_available(android 28, *)) {
stream->nSurfaceTexture = ASurfaceTexture_fromSurfaceTexture(env, surfaceTexture);
Expand All @@ -91,49 +91,50 @@ Stream* ExternalStreamManagerAndroid::acquire(jobject surfaceTexture) noexcept {
}

void ExternalStreamManagerAndroid::release(Stream* handle) noexcept {
EGLStream* stream = static_cast<EGLStream*>(handle);
EGLStream const* stream = static_cast<EGLStream*>(handle);
if (__builtin_available(android 28, *)) {
ASurfaceTexture_release(stream->nSurfaceTexture);
}
JNIEnv* const env = getEnvironment();
// use VirtualMachineEnv::getEnvironment() directly because we don't need to cache JNI methods here
JNIEnv* const env = mVm.getEnvironment();
assert_invariant(env); // we should have called attach() by now
env->DeleteGlobalRef(stream->jSurfaceTexture);
delete stream;
}

void ExternalStreamManagerAndroid::attach(Stream* handle, intptr_t tname) noexcept {
EGLStream* stream = static_cast<EGLStream*>(handle);
EGLStream const* stream = static_cast<EGLStream*>(handle);
if (__builtin_available(android 28, *)) {
// associate our GL texture to the SurfaceTexture
ASurfaceTexture* const aSurfaceTexture = stream->nSurfaceTexture;
if (UTILS_UNLIKELY(ASurfaceTexture_attachToGLContext(aSurfaceTexture, (uint32_t)tname))) {
if (UTILS_UNLIKELY(ASurfaceTexture_attachToGLContext(aSurfaceTexture, uint32_t(tname)))) {
// Unfortunately, before API 26 SurfaceTexture was always created in attached mode,
// so attachToGLContext can fail. We consider this the unlikely case, because
// this is how it should work.
// So now we have to detach the surfacetexture from its texture
ASurfaceTexture_detachFromGLContext(aSurfaceTexture);
// and finally, try attaching again
ASurfaceTexture_attachToGLContext(aSurfaceTexture, (uint32_t)tname);
ASurfaceTexture_attachToGLContext(aSurfaceTexture, uint32_t(tname));
}
} else {
JNIEnv* const env = getEnvironment();
assert_invariant(env); // we should have called attach() by now

// associate our GL texture to the SurfaceTexture
jobject jSurfaceTexture = stream->jSurfaceTexture;
env->CallVoidMethod(jSurfaceTexture, mSurfaceTextureClass_attachToGLContext, (jint)tname);
env->CallVoidMethod(jSurfaceTexture, mSurfaceTextureClass_attachToGLContext, jint(tname));
if (UTILS_UNLIKELY(env->ExceptionCheck())) {
// Unfortunately, before API 26 SurfaceTexture was always created in attached mode,
// so attachToGLContext can fail. We consider this the unlikely case, because
// this is how it should work.
env->ExceptionClear();

// so now we have to detach the surfacetexture from its texture
// so now we have to detach the SurfaceTexture from its texture
env->CallVoidMethod(jSurfaceTexture, mSurfaceTextureClass_detachFromGLContext);
VirtualMachineEnv::handleException(env);

// and finally, try attaching again
env->CallVoidMethod(jSurfaceTexture, mSurfaceTextureClass_attachToGLContext, (jint)tname);
env->CallVoidMethod(jSurfaceTexture, mSurfaceTextureClass_attachToGLContext, jint(tname));
VirtualMachineEnv::handleException(env);
}
}
Expand All @@ -144,20 +145,20 @@ void ExternalStreamManagerAndroid::detach(Stream* handle) noexcept {
if (__builtin_available(android 28, *)) {
ASurfaceTexture_detachFromGLContext(stream->nSurfaceTexture);
} else {
JNIEnv* const env = mVm.getEnvironment();
JNIEnv* const env = getEnvironment();
assert_invariant(env); // we should have called attach() by now
env->CallVoidMethod(stream->jSurfaceTexture, mSurfaceTextureClass_detachFromGLContext);
VirtualMachineEnv::handleException(env);
}
}

void ExternalStreamManagerAndroid::updateTexImage(Stream* handle, int64_t* timestamp) noexcept {
EGLStream* stream = static_cast<EGLStream*>(handle);
EGLStream const* stream = static_cast<EGLStream*>(handle);
if (__builtin_available(android 28, *)) {
ASurfaceTexture_updateTexImage(stream->nSurfaceTexture);
*timestamp = ASurfaceTexture_getTimestamp(stream->nSurfaceTexture);
} else {
JNIEnv* const env = mVm.getEnvironment();
JNIEnv* const env = getEnvironment();
assert_invariant(env); // we should have called attach() by now
env->CallVoidMethod(stream->jSurfaceTexture, mSurfaceTextureClass_updateTexImage);
VirtualMachineEnv::handleException(env);
Expand Down
11 changes: 8 additions & 3 deletions filament/backend/src/opengl/platforms/ExternalStreamManagerAndroid.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,19 @@

#include <backend/Platform.h>

#include <utils/compiler.h>

#if __has_include(<android/surface_texture.h>)
# include <android/surface_texture.h>
# include <android/surface_texture_jni.h>
#else
struct ASurfaceTexture;
typedef struct ASurfaceTexture ASurfaceTexture;
#endif

#include <jni.h>

#include <stdint.h>

namespace filament::backend {

/*
Expand Down Expand Up @@ -70,7 +75,8 @@ class ExternalStreamManagerAndroid {
ASurfaceTexture* nSurfaceTexture = nullptr;
};

inline JNIEnv* getEnvironment() noexcept {
// Must only be called from the backend thread
JNIEnv* getEnvironment() noexcept {
JNIEnv* env = mJniEnv;
if (UTILS_UNLIKELY(!env)) {
return getEnvironmentSlow();
Expand All @@ -85,7 +91,6 @@ class ExternalStreamManagerAndroid {
jmethodID mSurfaceTextureClass_attachToGLContext{};
jmethodID mSurfaceTextureClass_detachFromGLContext{};
};

} // namespace filament::backend

#endif //TNT_FILAMENT_BACKEND_OPENGL_ANDROID_EXTERNAL_STREAM_MANAGER_ANDROID_H

0 comments on commit 42a5757

Please sign in to comment.