Skip to content

Fix crash in OpenGLDriver::destroyStream#9801

Merged
z3moon merged 5 commits intomainfrom
zm/fix-destroy-stream
Mar 19, 2026
Merged

Fix crash in OpenGLDriver::destroyStream#9801
z3moon merged 5 commits intomainfrom
zm/fix-destroy-stream

Conversation

@z3moon
Copy link
Contributor

@z3moon z3moon commented Mar 16, 2026

Fixes a SIGSEGV crash that occurs during
ExternalStreamManagerAndroid::release() when users provide a custom Platform instance during Engine creation.

When a custom Platform is used, ExternalStreamManagerAndroid is instantiated on the application's thread. Its constructor was caching a C++ reference (VirtualMachineEnv& mVm) to the thread_local VirtualMachineEnv instance belonging to the application thread.

Later, when the Filament backend thread executed release(), it accessed this cached reference. This caused the backend thread to retrieve the JNIEnv* from the application thread's thread_local instance, and subsequently call env->DeleteGlobalRef(). A thread must use its own JNIEnv pointer, so using the application thread's pointer from the backend thread resulted in a crash.

BUGS=[489814416]

Fixes a SIGSEGV crash that occurs during
`ExternalStreamManagerAndroid::release()` when users provide a custom
Platform instance during Engine creation.

When a custom Platform is used, ExternalStreamManagerAndroid is
instantiated on the application's thread. Its constructor was caching a
C++ reference (`VirtualMachineEnv& mVm`) to the `thread_local`
VirtualMachineEnv instance belonging to the application thread.

Later, when the Filament backend thread executed `release()`, it
accessed this cached reference. This caused the backend thread to
retrieve the `JNIEnv*` from the application thread's `thread_local`
instance, and subsequently call `env->DeleteGlobalRef()`.  A thread must
use its own `JNIEnv` pointer, so using the application thread's pointer
from the backend thread resulted in a crash.

BUGS=[489814416]
@z3moon z3moon added the internal Issue/PR does not affect clients label Mar 16, 2026
Copy link
Contributor

@poweifeng poweifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a similar fix for PerformanceHIntManager here: cff9585

Can we do something similar here? (Like initialize ExternalStreamManagerAndroid on the backend thread).

@z3moon
Copy link
Contributor Author

z3moon commented Mar 16, 2026

there was a similar fix for PerformanceHIntManager here: cff9585

Can we do something similar here? (Like initialize ExternalStreamManagerAndroid on the backend thread).

In the current Filament design, a stream is created in the application thread via createStreamNative or createStreamAcquired, and they're destroyed in the backend thread via destroyStream. I think this is because the application thread (Java) should directly increase the global reference of the given surface texture instance. (it wouldn't be valid in the backend thread). So we should allow both threads to have its own JNIEnv.

UTILS_NOINLINE
JNIEnv* ExternalStreamManagerAndroid::getEnvironmentSlow() noexcept {
JNIEnv* const env = mVm.getEnvironment();
JNIEnv* const env = VirtualMachineEnv::get().getEnvironment();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VirtualMachineEnv::get() is documented as "must be called on the backend thread"
This is also the case for VirtualMachineEnv::getEnvironment().

I think what should happen instead is that ExternalStreamManager should always be created from the backend thread (this broke, guess, when we allowed custom Platforms).

And, we should use VirtualMachineEnv::getThreadEnvironment() to get a JNIEnv from any thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I moved the creation to the backend side.

@z3moon z3moon force-pushed the zm/fix-destroy-stream branch from 4e34202 to 4004224 Compare March 17, 2026 21:30
@z3moon z3moon force-pushed the zm/fix-destroy-stream branch from 4004224 to 567367a Compare March 17, 2026 21:31
@z3moon z3moon enabled auto-merge (squash) March 19, 2026 01:49
@z3moon z3moon merged commit f24db46 into main Mar 19, 2026
18 checks passed
@z3moon z3moon deleted the zm/fix-destroy-stream branch March 19, 2026 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Issue/PR does not affect clients

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants