android: Replace env->NewGlobalRef with NoDestructor wrapper#10783
android: Replace env->NewGlobalRef with NoDestructor wrapper#10783kjyoun wants to merge 5 commits into
Conversation
- Refactor starboard/android/shared/video_window.cc to replace direct env->NewGlobalRef call with starboard::NoDestructor<jni_zero::ScopedJavaGlobalRef<jobject>>. - This ensures safe lifecycle management of the global video surface JNI reference and avoids potential shutdown crashes. - Add starboard::NoDestructor utility to starboard/common, mirroring base::NoDestructor. - Add unit tests for starboard::NoDestructor in no_destructor_test.cc. BUG=b/520007952 TAG=agy CONV=5d7aa66e-ca8b-4eab-bf9f-38fb21320b8c
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
13dd015 to
c159a30
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a NoDestructor<T> utility class to prevent the execution of non-trivial destructors for static and global variables, accompanied by comprehensive unit tests. Additionally, it refactors the global video surface management in video_window.cc to utilize jni_zero::ScopedJavaGlobalRef wrapped in NoDestructor. The review feedback recommends using std::launder in NoDestructor::get() to comply with C++17 strict aliasing rules and prevent undefined behavior. It also suggests simplifying the JNI reference resetting logic in video_window.cc since ScopedJavaGlobalRef::Reset automatically handles releasing the previous reference.
There was a problem hiding this comment.
Code Review
This pull request introduces a NoDestructor helper class to starboard/common along with its unit tests, and utilizes it in video_window.cc to manage a global Java reference safely using jni_zero::ScopedJavaGlobalRef. The feedback suggests simplifying the video surface reset logic by removing a redundant null check, as Reset() is safe to call on empty references.
Issue: 520007952
Issue: 509619283