Track ANativeActivity pointer lifetime more carefully

Once `on_destroy()` returns then the `NativeActivity.java` code will call an
`unloadNativeCode` native method that will `delete` the `ANativeActivity`
and invalidate any pointers we hold.

Considering the possibility that an `AndroidApp` could be retained
beyond the lifetime of the original `NativeActivity`, this ensures we
always hold the `WaitableNativeActivityState::mutex` before
dereferencing this pointer and ensures we clear the pointer before
returning from `on_destroy` so we're also able to perform `null` pointer
checks before dereferencing.

Considering that `AndroidApp::vm_as_ptr` previously depended on
dereferencing the `ANativeActivity`, this updates it to instead use
`JavaVM::singleton()` which we guarantee will be initialized before the
`AndroidApp` is created.

Considering that `AndroidApp::activity_as_ptr()` promises to return a
global reference that remains valid for the lifetime of the
`AndroidApp`, but the `ANativeActivity::clazz` reference is deleted
after `on_destroy()` returns, we now create our own `Global` reference
for the `Activity` that is owned by `AndroidAppInner`.

android-activity/CHANGELOG.md

@@ -14,14 +14,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `AndroidApp::java_main_looper()` gives access to the `ALooper` for the Java main / UI thread ([#198](https://github.com/rust-mobile/android-activity/pull/198))
 - `AndroidApp::run_on_java_main_thread()` can be used to run boxed closures on the Java main / UI thread ([#232](https://github.com/rust-mobile/android-activity/pull/232))
 
-### Fixed
-
-- *Safety* `AndroidApp::asset_manager()` returns an `AssetManager` that has a safe `'static` lifetime that's not invalidated when `android_main()` returns [#233](https://github.com/rust-mobile/android-activity/pull/233)
-
 ### Changed
+
 - rust-version bumped to 1.85.0 ([#193](https://github.com/rust-mobile/android-activity/pull/193), [#219](https://github.com/rust-mobile/android-activity/pull/219))
 - GameActivity updated to 4.0.0 (requires the corresponding 4.0.0 `.aar` release from Google) ([#191](https://github.com/rust-mobile/android-activity/pull/191))
 
+### Fixed
+
+- *Safety* `AndroidApp::asset_manager()` returns an `AssetManager` that has a safe `'static` lifetime that's not invalidated when `android_main()` returns ([#233](https://github.com/rust-mobile/android-activity/pull/233))
+- *Safety* The `native-activity` backend clears its `ANativeActivity` ptr after `onDestroy` and `AndroidApp` remains safe to access after `android_main()` returns ([#234](https://github.com/rust-mobile/android-activity/pull/234))
+- *Safety* `AndroidApp::activity_as_ptr()` returns a pointer to a global reference that remains valid until `AndroidApp` is dropped, instead of the `ANativeActivity`'s `clazz` pointer which is only guaranteed to be valid until `onDestroy` returns (`native-activity` backend) ([#234](https://github.com/rust-mobile/android-activity/pull/234))
+
 ## [0.6.0] - 2024-04-26
 
 ### Changed

android-activity/src/game_activity/mod.rs

@@ -316,11 +316,6 @@ pub struct AndroidAppInner {
 }
 
 impl AndroidAppInner {
-    pub fn vm_as_ptr(&self) -> *mut c_void {
-        let app_ptr = self.native_app.as_ptr();
-        unsafe { (*(*app_ptr).activity).vm as _ }
-    }
-
     pub fn activity_as_ptr(&self) -> *mut c_void {
         let app_ptr = self.native_app.as_ptr();
         unsafe { (*(*app_ptr).activity).javaGameActivity as _ }

android-activity/src/lib.rs

@@ -121,6 +121,7 @@ use std::sync::RwLock;
 use std::time::Duration;
 
 use bitflags::bitflags;
+use jni::vm::JavaVM;
 use libc::c_void;
 
 use ndk::asset::AssetManager;
@@ -596,29 +597,39 @@ impl AndroidApp {
     /// [`jni`]: https://crates.io/crates/jni
     /// [`JavaVM`]: https://docs.rs/jni/latest/jni/struct.JavaVM.html
     pub fn vm_as_ptr(&self) -> *mut c_void {
-        self.inner.read().unwrap().vm_as_ptr()
+        JavaVM::singleton().unwrap().get_raw() as _
     }
 
-    /// Returns a JNI object reference for this application's JVM `Activity` as a pointer
+    /// Returns an (*unowned*) JNI global object reference for this
+    /// application's JVM `Activity` as a pointer
     ///
-    /// If you use the [`jni`] crate you can cast this as a `JObject` reference via:
+    /// If you use the [`jni`] crate you can cast this as a `JObject` reference
+    /// via:
     /// ```no_run
     /// # use jni::objects::JObject;
     /// # use jni::refs::Global;
     /// # fn use_jni(env: &jni::Env, app: &android_activity::AndroidApp) -> jni::errors::Result<()> {
     /// let raw_activity_global = app.activity_as_ptr() as jni::sys::jobject;
-    /// // SAFETY: The pointer is valid as long as `app` is valid.
+    /// // SAFETY: The reference / pointer is valid as long as `app` is valid
     /// let activity = unsafe { env.as_cast_raw::<Global<JObject>>(&raw_activity_global)? };
     /// # Ok(()) }
     /// ```
     ///
     /// # JNI Safety
     ///
-    /// Note that the object reference will be a JNI global reference, not a
-    /// local reference and it should not be deleted. Don't wrap the reference
-    /// in an [`Auto`] which would try to explicitly delete the reference
-    /// when dropped. Similarly, don't wrap the reference as a [`Global`]
-    /// which would also try to explicitly delete the reference when dropped.
+    /// Note that the returned reference will be a JNI global reference *that
+    /// you do not own*.
+    /// - Don't wrap the reference as a [`Global`] which would try to delete the
+    ///   reference when dropped.
+    /// - Don't wrap the reference in an [`Auto`] which would treat the
+    ///   reference like a local reference and try to delete it when dropped.
+    ///
+    /// The reference is only guaranteed to be valid until you drop the
+    /// [`AndroidApp`].
+    ///
+    /// **Warning:** Don't assume the returned reference has a `'static` lifetime
+    /// since it's possible for `android_main()` to run multiple times over the
+    /// lifetime of an application with a new `AndroidApp` instance each time.
     ///
     /// [`jni`]: https://crates.io/crates/jni
     /// [`Auto`]: https://docs.rs/jni/latest/jni/refs/struct.Auto.html

android-activity/src/native_activity/glue.rs

@@ -75,8 +75,6 @@ pub enum State {
 
 #[derive(Debug)]
 pub struct WaitableNativeActivityState {
-    pub activity: *mut ndk_sys::ANativeActivity,
-
     pub mutex: Mutex<NativeActivityState>,
     pub cond: Condvar,
 }
@@ -210,6 +208,29 @@ pub enum NativeThreadState {
 
 #[derive(Debug)]
 pub struct NativeActivityState {
+    /// Set as soon as the Java main thread notifies us of an `onDestroyed`
+    /// callback.
+    pub destroyed: bool,
+
+    /// The `ANativeActivity` associated with the NativeActivity instance
+    ///
+    /// # Safety
+    ///
+    /// This pointer will be reset to `null` when the NativeActivity is
+    /// destroyed.
+    ///
+    /// Keep in mind that `NativeActivityState` is ref-counted and can
+    /// potentially out-last an `onDestroy` callback where we may reset this to
+    /// be a null pointer!
+    ///
+    /// For example:
+    /// - An application could put an `AndroidApp` into a global `'static` and
+    ///   keep it alive beyond `android_main`
+    /// - An application could schedule a callback to run on the Java main
+    ///   thread with an `AndroidApp` clone and by the time it runs then the
+    ///   associated `ANativeActivity` could have been destroyed.
+    pub activity: *mut ndk_sys::ANativeActivity,
+
     pub msg_read: libc::c_int,
     pub msg_write: libc::c_int,
     pub config: ConfigurationRef,
@@ -222,9 +243,6 @@ pub struct NativeActivityState {
     pub thread_state: NativeThreadState,
     pub app_has_saved_state: bool,
 
-    /// Set as soon as the Java main thread notifies us of an
-    /// `onDestroyed` callback.
-    pub destroyed: bool,
     pub pending_input_queue: *mut ndk_sys::AInputQueue,
     pub pending_window: Option<NativeWindow>,
 }
@@ -357,8 +375,8 @@ impl WaitableNativeActivityState {
         };
 
         Self {
-            activity,
             mutex: Mutex::new(NativeActivityState {
+                activity,
                 msg_read: msgpipe[0],
                 msg_write: msgpipe[1],
                 config,
@@ -392,6 +410,11 @@ impl WaitableNativeActivityState {
             guard.msg_read = -1;
             libc::close(guard.msg_write);
             guard.msg_write = -1;
+
+            // The last thing that `NativeActivity` `onDestroy` does is to call a
+            // native method (`unloadNativeCode`) which will `delete` the
+            // `ANativeActivity` instance.
+            guard.activity = ptr::null_mut();
         }
     }
 
@@ -605,7 +628,7 @@ impl WaitableNativeActivityState {
             AppCmd::ConfigChanged => {
                 let guard = self.mutex.lock().unwrap();
                 let config = ndk_sys::AConfiguration_new();
-                ndk_sys::AConfiguration_fromAssetManager(config, (*self.activity).assetManager);
+                ndk_sys::AConfiguration_fromAssetManager(config, (*guard.activity).assetManager);
                 let config = Configuration::from_ptr(NonNull::new_unchecked(config));
                 guard.config.replace(config);
                 log::debug!("Config: {:#?}", guard.config);
@@ -896,6 +919,8 @@ fn rust_glue_entry(
             Some(16),
             |env| -> jni::errors::Result<()> {
                 // SAFETY: We know jni_activity is a valid JNI global ref to an Activity instance
+                // that will remain valid until `onDestroy` is handled (not possible until we start
+                // `android_main()`).
                 let jni_activity = unsafe { env.as_cast_raw::<Global<JObject>>(&jni_activity)? };
 
                 let (app_asset_manager, main_callbacks) =
@@ -910,11 +935,12 @@ fn rust_glue_entry(
                     };
 
                 let app = AndroidApp::new(
-                    rust_glue.clone(),
                     jvm.clone(),
-                    app_asset_manager,
                     main_looper,
                     main_callbacks,
+                    app_asset_manager,
+                    rust_glue.clone(),
+                    &jni_activity,
                 );
 
                 rust_glue.notify_main_thread_running();

android-activity/src/native_activity/mod.rs

@@ -5,6 +5,7 @@ use std::ptr;
 use std::sync::{Arc, Mutex, RwLock, Weak};
 use std::time::Duration;
 
+use jni::objects::JObject;
 use jni::JavaVM;
 use libc::c_void;
 use log::{error, trace};
@@ -63,11 +64,12 @@ impl StateLoader<'_> {
 
 impl AndroidApp {
     pub(crate) fn new(
-        native_activity: NativeActivityGlue,
         jvm: JavaVM,
-        app_asset_manager: AssetManager,
         main_looper: ndk::looper::ForeignLooper,
         main_callbacks: MainCallbacks,
+        app_asset_manager: AssetManager,
+        native_activity: NativeActivityGlue,
+        jni_activity: &JObject,
     ) -> Self {
         jvm.with_local_frame(10, |env| -> jni::errors::Result<_> {
             if let Err(err) = crate::input::jni_init(env) {
@@ -80,16 +82,25 @@ impl AndroidApp {
                 );
                 ndk::looper::ForeignLooper::from_ptr(ptr::NonNull::new(ptr).unwrap())
             };
+
+            // The global reference in `ANativeActivity` is only guaranteed to be valid until
+            // `onDestroy` returns, so we create our own global reference that we can guarantee will
+            // remain valid until `AndroidApp` is dropped.
+            let activity = env
+                .new_global_ref(jni_activity)
+                .expect("Failed to create global ref for Activity instance");
+
             let app = Self {
                 inner: Arc::new(RwLock::new(AndroidAppInner {
                     jvm: jvm.clone(),
-                    native_activity,
-                    looper,
                     main_looper,
                     main_callbacks,
+                    app_asset_manager,
+                    native_activity,
+                    activity,
+                    looper,
                     key_maps: Mutex::new(HashMap::new()),
                     input_receiver: Mutex::new(None),
-                    app_asset_manager,
                 })),
             };
 
@@ -122,6 +133,8 @@ pub(crate) struct AndroidAppInner {
 
     pub(crate) native_activity: NativeActivityGlue,
 
+    activity: jni::refs::Global<jni::objects::JObject<'static>>,
+
     main_callbacks: MainCallbacks,
 
     /// Looper associated with the Rust `android_main` thread
@@ -151,17 +164,11 @@ pub(crate) struct AndroidAppInner {
 }
 
 impl AndroidAppInner {
-    pub(crate) fn vm_as_ptr(&self) -> *mut c_void {
-        unsafe { (*self.native_activity.activity).vm as _ }
-    }
-
     pub(crate) fn activity_as_ptr(&self) -> *mut c_void {
-        // "clazz" is a completely bogus name; this is the _instance_ not class pointer
-        unsafe { (*self.native_activity.activity).clazz as _ }
-    }
-
-    pub(crate) fn native_activity(&self) -> *const ndk_sys::ANativeActivity {
-        self.native_activity.activity
+        // Note: The global reference in `ANativeActivity::clazz` (misnomer for instance reference)
+        // is only guaranteed to be valid until `onDestroy` returns, so we have our own global
+        // reference that we can instead guarantee will remain valid until `AndroidApp` is dropped.
+        self.activity.as_raw() as *mut c_void
     }
 
     pub(crate) fn looper_as_ptr(&self) -> *mut ndk_sys::ALooper {
@@ -336,7 +343,13 @@ impl AndroidAppInner {
         add_flags: WindowManagerFlags,
         remove_flags: WindowManagerFlags,
     ) {
-        let na = self.native_activity();
+        let guard = self.native_activity.mutex.lock().unwrap();
+        let na = guard.activity;
+        if na.is_null() {
+            log::error!("Can't set window flags after NativeActivity has been destroyed");
+            return;
+        }
+
         let na_mut = na as *mut ndk_sys::ANativeActivity;
         unsafe {
             ndk_sys::ANativeActivity_setWindowFlags(
@@ -349,7 +362,12 @@ impl AndroidAppInner {
 
     // TODO: move into a trait
     pub fn show_soft_input(&self, show_implicit: bool) {
-        let na = self.native_activity();
+        let guard = self.native_activity.mutex.lock().unwrap();
+        let na = guard.activity;
+        if na.is_null() {
+            log::error!("Can't show soft input after NativeActivity has been destroyed");
+            return;
+        }
         unsafe {
             let flags = if show_implicit {
                 ndk_sys::ANATIVEACTIVITY_SHOW_SOFT_INPUT_IMPLICIT
@@ -362,7 +380,12 @@ impl AndroidAppInner {
 
     // TODO: move into a trait
     pub fn hide_soft_input(&self, hide_implicit_only: bool) {
-        let na = self.native_activity();
+        let guard = self.native_activity.mutex.lock().unwrap();
+        let na = guard.activity;
+        if na.is_null() {
+            log::error!("Can't hide soft input after NativeActivity has been destroyed");
+            return;
+        }
         unsafe {
             let flags = if hide_implicit_only {
                 ndk_sys::ANATIVEACTIVITY_HIDE_SOFT_INPUT_IMPLICIT_ONLY
@@ -447,17 +470,32 @@ impl AndroidAppInner {
     }
 
     pub fn internal_data_path(&self) -> Option<std::path::PathBuf> {
-        let na = self.native_activity();
+        let guard = self.native_activity.mutex.lock().unwrap();
+        let na = guard.activity;
+        if na.is_null() {
+            log::error!("Can't get internal data path after NativeActivity has been destroyed");
+            return None;
+        }
         unsafe { util::try_get_path_from_ptr((*na).internalDataPath) }
     }
 
     pub fn external_data_path(&self) -> Option<std::path::PathBuf> {
-        let na = self.native_activity();
+        let guard = self.native_activity.mutex.lock().unwrap();
+        let na = guard.activity;
+        if na.is_null() {
+            log::error!("Can't get external data path after NativeActivity has been destroyed");
+            return None;
+        }
         unsafe { util::try_get_path_from_ptr((*na).externalDataPath) }
     }
 
     pub fn obb_path(&self) -> Option<std::path::PathBuf> {
-        let na = self.native_activity();
+        let guard = self.native_activity.mutex.lock().unwrap();
+        let na = guard.activity;
+        if na.is_null() {
+            log::error!("Can't get OBB path after NativeActivity has been destroyed");
+            return None;
+        }
         unsafe { util::try_get_path_from_ptr((*na).obbPath) }
     }
 }