Call Activity.finish() when android_main returns

Calling Activity.finish() is what ensures the Activity will get
gracefully destroyed, including calling the Activity's onDestroy
method.

Fixes: #67

android-activity/CHANGELOG.md

@@ -3,6 +3,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Changed
+- The `Activity.finish()` method is now called when `android_main` returns so the `Activity` will be destroyed ([#67](https://github.com/rust-mobile/android-activity/issues/67))
 
 ## [0.4.1] - 2022-02-16
 ### Added

android-activity/src/game_activity/mod.rs

@@ -6,6 +6,7 @@ use std::io::{BufRead, BufReader};
 use std::marker::PhantomData;
 use std::ops::Deref;
 use std::os::unix::prelude::*;
+use std::panic::catch_unwind;
 use std::ptr::NonNull;
 use std::sync::{Arc, RwLock};
 use std::time::Duration;
@@ -23,7 +24,7 @@ use ndk::asset::AssetManager;
 use ndk::configuration::Configuration;
 use ndk::native_window::NativeWindow;
 
-use crate::util::{abort_on_panic, android_log};
+use crate::util::{abort_on_panic, android_log, log_panic};
 use crate::{
     util, AndroidApp, ConfigurationRef, InputStatus, MainEvent, PollEvent, Rect, WindowManagerFlags,
 };
@@ -603,7 +604,8 @@ extern "Rust" {
 // `app_main` function. This is run on a dedicated thread spawned
 // by android_native_app_glue.
 #[no_mangle]
-pub unsafe extern "C" fn _rust_glue_entry(app: *mut ffi::android_app) {
+#[allow(unused_unsafe)] // Otherwise rust 1.64 moans about using unsafe{} in unsafe functions
+pub unsafe extern "C" fn _rust_glue_entry(native_app: *mut ffi::android_app) {
     abort_on_panic(|| {
         // Maybe make this stdout/stderr redirection an optional / opt-in feature?...
         let mut logpipe: [RawFd; 2] = Default::default();
@@ -627,34 +629,51 @@ pub unsafe extern "C" fn _rust_glue_entry(app: *mut ffi::android_app) {
             }
         });
 
-        let jvm: *mut JavaVM = (*(*app).activity).vm;
-        let activity: jobject = (*(*app).activity).javaGameActivity;
-        ndk_context::initialize_android_context(jvm.cast(), activity.cast());
+        let jvm = unsafe {
+            let jvm = (*(*native_app).activity).vm;
+            let activity: jobject = (*(*native_app).activity).javaGameActivity;
+            ndk_context::initialize_android_context(jvm.cast(), activity.cast());
 
-        let app = AndroidApp::from_ptr(NonNull::new(app).unwrap());
+            // Since this is a newly spawned thread then the JVM hasn't been attached
+            // to the thread yet. Attach before calling the applications main function
+            // so they can safely make JNI calls
+            let mut jenv_out: *mut core::ffi::c_void = std::ptr::null_mut();
+            if let Some(attach_current_thread) = (*(*jvm)).AttachCurrentThread {
+                attach_current_thread(jvm, &mut jenv_out, std::ptr::null_mut());
+            }
 
-        // Since this is a newly spawned thread then the JVM hasn't been attached
-        // to the thread yet. Attach before calling the applications main function
-        // so they can safely make JNI calls
-        let mut jenv_out: *mut core::ffi::c_void = std::ptr::null_mut();
-        if let Some(attach_current_thread) = (*(*jvm)).AttachCurrentThread {
-            attach_current_thread(jvm, &mut jenv_out, std::ptr::null_mut());
+            jvm
+        };
+
+        unsafe {
+            let app = AndroidApp::from_ptr(NonNull::new(native_app).unwrap());
+
+            // We want to specifically catch any panic from the application's android_main
+            // so we can finish + destroy the Activity gracefully via the JVM
+            catch_unwind(|| {
+                // XXX: If we were in control of the Java Activity subclass then
+                // we could potentially run the android_main function via a Java native method
+                // springboard (e.g. call an Activity subclass method that calls a jni native
+                // method that then just calls android_main()) that would make sure there was
+                // a Java frame at the base of our call stack which would then be recognised
+                // when calling FindClass to lookup a suitable classLoader, instead of
+                // defaulting to the system loader. Without this then it's difficult for native
+                // code to look up non-standard Java classes.
+                android_main(app);
+            })
+            .unwrap_or_else(|panic| log_panic(panic));
+
+            // Let JVM know that our Activity can be destroyed before detaching from the JVM
+            //
+            // "Note that this method can be called from any thread; it will send a message
+            //  to the main thread of the process where the Java finish call will take place"
+            ffi::GameActivity_finish((*native_app).activity);
+
+            if let Some(detach_current_thread) = (*(*jvm)).DetachCurrentThread {
+                detach_current_thread(jvm);
+            }
+
+            ndk_context::release_android_context();
         }
-
-        // XXX: If we were in control of the Java Activity subclass then
-        // we could potentially run the android_main function via a Java native method
-        // springboard (e.g. call an Activity subclass method that calls a jni native
-        // method that then just calls android_main()) that would make sure there was
-        // a Java frame at the base of our call stack which would then be recognised
-        // when calling FindClass to lookup a suitable classLoader, instead of
-        // defaulting to the system loader. Without this then it's difficult for native
-        // code to look up non-standard Java classes.
-        android_main(app);
-
-        if let Some(detach_current_thread) = (*(*jvm)).DetachCurrentThread {
-            detach_current_thread(jvm);
-        }
-
-        ndk_context::release_android_context();
     })
 }

android-activity/src/native_activity/glue.rs

@@ -8,6 +8,7 @@ use std::{
     io::{BufRead, BufReader},
     ops::Deref,
     os::unix::prelude::{FromRawFd, RawFd},
+    panic::catch_unwind,
     ptr::{self, NonNull},
     sync::{Arc, Condvar, Mutex, Weak},
 };
@@ -15,7 +16,11 @@ use std::{
 use log::Level;
 use ndk::{configuration::Configuration, input_queue::InputQueue, native_window::NativeWindow};
 
-use crate::{util::abort_on_panic, util::android_log, ConfigurationRef};
+use crate::{
+    util::android_log,
+    util::{abort_on_panic, log_panic},
+    ConfigurationRef,
+};
 
 use super::{AndroidApp, Rect};
 
@@ -803,6 +808,7 @@ unsafe extern "C" fn on_content_rect_changed(
 
 /// This is the native entrypoint for our cdylib library that `ANativeActivity` will look for via `dlsym`
 #[no_mangle]
+#[allow(unused_unsafe)] // Otherwise rust 1.64 moans about using unsafe{} in unsafe functions
 extern "C" fn ANativeActivity_onCreate(
     activity: *mut ndk_sys::ANativeActivity,
     saved_state: *const libc::c_void,
@@ -874,15 +880,26 @@ extern "C" fn ANativeActivity_onCreate(
             rust_glue.notify_main_thread_running();
 
             unsafe {
-                // XXX: If we were in control of the Java Activity subclass then
-                // we could potentially run the android_main function via a Java native method
-                // springboard (e.g. call an Activity subclass method that calls a jni native
-                // method that then just calls android_main()) that would make sure there was
-                // a Java frame at the base of our call stack which would then be recognised
-                // when calling FindClass to lookup a suitable classLoader, instead of
-                // defaulting to the system loader. Without this then it's difficult for native
-                // code to look up non-standard Java classes.
-                android_main(app);
+                // We want to specifically catch any panic from the application's android_main
+                // so we can finish + destroy the Activity gracefully via the JVM
+                catch_unwind(|| {
+                    // XXX: If we were in control of the Java Activity subclass then
+                    // we could potentially run the android_main function via a Java native method
+                    // springboard (e.g. call an Activity subclass method that calls a jni native
+                    // method that then just calls android_main()) that would make sure there was
+                    // a Java frame at the base of our call stack which would then be recognised
+                    // when calling FindClass to lookup a suitable classLoader, instead of
+                    // defaulting to the system loader. Without this then it's difficult for native
+                    // code to look up non-standard Java classes.
+                    android_main(app);
+                })
+                .unwrap_or_else(|panic| log_panic(panic));
+
+                // Let JVM know that our Activity can be destroyed before detaching from the JVM
+                //
+                // "Note that this method can be called from any thread; it will send a message
+                //  to the main thread of the process where the Java finish call will take place"
+                ndk_sys::ANativeActivity_finish(activity);
 
                 if let Some(detach_current_thread) = (*(*jvm)).DetachCurrentThread {
                     detach_current_thread(jvm);

android-activity/src/util.rs

@@ -31,6 +31,25 @@ pub(crate) fn android_log(level: Level, tag: &CStr, msg: &CStr) {
     }
 }
 
+pub(crate) fn log_panic(panic: Box<dyn std::any::Any + Send>) {
+    let rust_panic = unsafe { CStr::from_bytes_with_nul_unchecked(b"RustPanic\0") };
+
+    if let Some(panic) = panic.downcast_ref::<String>() {
+        if let Ok(msg) = CString::new(panic.clone()) {
+            android_log(Level::Error, rust_panic, &msg);
+        }
+    } else if let Ok(panic) = panic.downcast::<&str>() {
+        if let Ok(msg) = CString::new(*panic) {
+            android_log(Level::Error, rust_panic, &msg);
+        }
+    } else {
+        let unknown_panic = unsafe { CStr::from_bytes_with_nul_unchecked(b"UnknownPanic\0") };
+        android_log(Level::Error, unknown_panic, unsafe {
+            CStr::from_bytes_with_nul_unchecked(b"\0")
+        });
+    }
+}
+
 /// Run a closure and abort the program if it panics.
 ///
 /// This is generally used to ensure Rust callbacks won't unwind past the JNI boundary, which leads
@@ -45,26 +64,7 @@ pub(crate) fn abort_on_panic<R>(f: impl FnOnce() -> R) -> R {
         //
         // Just in case our attempt to log a panic could itself cause a panic we use a
         // second catch_unwind here.
-        let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
-            // Try logging the panic, but abort if that fails.
-            let rust_panic = unsafe { CStr::from_bytes_with_nul_unchecked(b"RustPanic\0") };
-
-            if let Some(panic) = panic.downcast_ref::<String>() {
-                if let Ok(msg) = CString::new(panic.clone()) {
-                    android_log(Level::Error, rust_panic, &msg);
-                }
-            } else if let Ok(panic) = panic.downcast::<&str>() {
-                if let Ok(msg) = CString::new(*panic) {
-                    android_log(Level::Error, rust_panic, &msg);
-                }
-            } else {
-                let unknown_panic =
-                    unsafe { CStr::from_bytes_with_nul_unchecked(b"UnknownPanic\0") };
-                android_log(Level::Error, unknown_panic, unsafe {
-                    CStr::from_bytes_with_nul_unchecked(b"\0")
-                });
-            }
-        }));
+        let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| log_panic(panic)));
         std::process::abort();
     })
 }