diff --git a/android-activity/CHANGELOG.md b/android-activity/CHANGELOG.md index 9467b3d..837efd8 100644 --- a/android-activity/CHANGELOG.md +++ b/android-activity/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - *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)) +- *Safety* The `game-activity` backend clears its `android_app` ptr after `onDestroy` and `AndroidApp` remains safe to access after `android_main()` returns ([#236](https://github.com/rust-mobile/android-activity/pull/236)) ## [0.6.0] - 2024-04-26 diff --git a/android-activity/src/game_activity/mod.rs b/android-activity/src/game_activity/mod.rs index 0ca8105..1f3f150 100644 --- a/android-activity/src/game_activity/mod.rs +++ b/android-activity/src/game_activity/mod.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; use std::marker::PhantomData; -use std::ops::Deref; use std::panic::catch_unwind; use std::ptr; use std::ptr::NonNull; @@ -51,37 +50,43 @@ pub struct StateSaver<'a> { impl<'a> StateSaver<'a> { pub fn store(&self, state: &'a [u8]) { - // android_native_app_glue specifically expects savedState to have been allocated - // via libc::malloc since it will automatically handle freeing the data once it - // has been handed over to the Java Activity / main thread. - unsafe { - let app_ptr = self.app.native_app.as_ptr(); - - // In case the application calls store() multiple times for some reason we - // make sure to free any pre-existing state... - if !(*app_ptr).savedState.is_null() { - libc::free((*app_ptr).savedState); - (*app_ptr).savedState = ptr::null_mut(); - (*app_ptr).savedStateSize = 0; + self.app.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + // Could probably be a panic since it shouldn't be possible to retain a `StateSaver` + // long enough for the `GameActivity` to be destroyed. + log::error!("Spurious attempt to save state after GameActivity was destroyed"); + return; } + // android_native_app_glue specifically expects savedState to have been allocated + // via libc::malloc since it will automatically handle freeing the data once it + // has been handed over to the Java Activity / main thread. + unsafe { + // In case the application calls store() multiple times for some reason we + // make sure to free any pre-existing state... + if !(*app_ptr).savedState.is_null() { + libc::free((*app_ptr).savedState); + (*app_ptr).savedState = ptr::null_mut(); + (*app_ptr).savedStateSize = 0; + } - let buf = libc::malloc(state.len()); - if buf.is_null() { - panic!("Failed to allocate save_state buffer"); + let buf = libc::malloc(state.len()); + if buf.is_null() { + panic!("Failed to allocate save_state buffer"); + } + + // Since it's a byte array there's no special alignment requirement here. + // + // Since we re-define `buf` we ensure it's not possible to access the buffer + // via its original pointer for the lifetime of the slice. + { + let buf: &mut [u8] = std::slice::from_raw_parts_mut(buf.cast(), state.len()); + buf.copy_from_slice(state); + } + + (*app_ptr).savedState = buf; + (*app_ptr).savedStateSize = state.len() as _; } - - // Since it's a byte array there's no special alignment requirement here. - // - // Since we re-define `buf` we ensure it's not possible to access the buffer - // via its original pointer for the lifetime of the slice. - { - let buf: &mut [u8] = std::slice::from_raw_parts_mut(buf.cast(), state.len()); - buf.copy_from_slice(state); - } - - (*app_ptr).savedState = buf; - (*app_ptr).savedStateSize = state.len() as _; - } + }); } } @@ -91,29 +96,37 @@ pub struct StateLoader<'a> { } impl StateLoader<'_> { pub fn load(&self) -> Option> { - unsafe { - let app_ptr = self.app.native_app.as_ptr(); - if !(*app_ptr).savedState.is_null() && (*app_ptr).savedStateSize > 0 { - let buf: &mut [u8] = std::slice::from_raw_parts_mut( - (*app_ptr).savedState.cast(), - (*app_ptr).savedStateSize, - ); - let state = buf.to_vec(); - Some(state) - } else { - None + self.app.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + // Could probably be a panic since it shouldn't be possible to retain a `StateLoader` + // long enough for the `GameActivity` to be destroyed. + log::error!("Spurious attempt to load state after GameActivity was destroyed"); + return None; } - } + unsafe { + if !(*app_ptr).savedState.is_null() && (*app_ptr).savedStateSize > 0 { + let buf: &mut [u8] = std::slice::from_raw_parts_mut( + (*app_ptr).savedState.cast(), + (*app_ptr).savedStateSize, + ); + let state = buf.to_vec(); + Some(state) + } else { + None + } + } + }) } } impl AndroidApp { pub(crate) fn new( - ptr: NonNull, jvm: jni::JavaVM, - app_asset_manager: AssetManager, main_looper: ndk::looper::ForeignLooper, main_callbacks: MainCallbacks, + app_asset_manager: AssetManager, + game_activity_ptr: *mut ffi::android_app, + jni_activity: &JObject, ) -> Self { // We attach to the thread before creating the AndroidApp jvm.with_local_frame(10, |env| -> jni::errors::Result<_> { @@ -125,20 +138,37 @@ impl AndroidApp { // and need to keep in mind that the Drop handler is going to call // AConfiguration_delete() let config = unsafe { - Configuration::clone_from_ptr(NonNull::new_unchecked((*ptr.as_ptr()).config)) + Configuration::clone_from_ptr(NonNull::new_unchecked((*game_activity_ptr).config)) }; + // The global reference in `android_app` 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"); + + // In order to support `AndroidApp::create_waker()` we need to acquire our own reference + // to the android_main thread looper because the GameActivity glue code will release + // it's own reference when handling the APP_CMD_DESTROY event, which could happen while + // we still have a live AndroidApp instance. + let looper = unsafe { + let ptr = (*game_activity_ptr).looper; + ndk::looper::ForeignLooper::from_ptr(ptr::NonNull::new(ptr).unwrap()) + }; Ok(Self { inner: Arc::new(RwLock::new(AndroidAppInner { jvm: jvm.clone(), - native_app: NativeAppGlue { ptr }, - config: ConfigurationRef::new(config), - native_window: Default::default(), main_looper, main_callbacks, + app_asset_manager, + game_activity: GameActivityGlue::new(game_activity_ptr), + activity, + looper, + config: ConfigurationRef::new(config), + native_window: Default::default(), key_maps: Mutex::new(HashMap::new()), input_receiver: Mutex::new(None), - app_asset_manager, })), }) }) @@ -147,64 +177,120 @@ impl AndroidApp { } #[derive(Debug, Clone)] -struct NativeAppGlue { - ptr: NonNull, +struct GameActivityGlue { + game_activity_app: Arc>, } -impl Deref for NativeAppGlue { - type Target = NonNull; - fn deref(&self) -> &Self::Target { - &self.ptr +impl GameActivityGlue { + fn new(game_activity_app: *mut ffi::android_app) -> Self { + Self { + game_activity_app: Arc::new(Mutex::new(game_activity_app)), + } + } + + fn locked_app(&self) -> std::sync::MutexGuard<'_, *mut ffi::android_app> { + self.game_activity_app.lock().unwrap() + } + + /// Access the GameActivity `android_app` glue with the guarantee that the + /// pointer will remain consistent for the duration of the closure because + /// the same lock must be held in order to handle the `APP_CMD_DESTROY` + /// event that invalidates the pointer. + /// + /// *Important*: The app pointer may _already_ be `null` (indicating that + /// the GameActivity has been destroyed) and must be checked by the caller + /// before dereferencing. + fn with_locked_app(&self, f: F) -> R + where + F: FnOnce(*mut ffi::android_app) -> R, + { + let app = self.locked_app(); + f(*app) + } + + /// Called when handling the `APP_CMD_DESTROY` event to clear our retained + /// pointer to the GameActivity `android_app` glue so that we don't + /// accidentally access it after it's been freed. + fn clear_app(&self) { + let mut app = self.locked_app(); + *app = ptr::null_mut(); } } -unsafe impl Send for NativeAppGlue {} -unsafe impl Sync for NativeAppGlue {} -impl NativeAppGlue { +unsafe impl Send for GameActivityGlue {} +unsafe impl Sync for GameActivityGlue {} + +impl GameActivityGlue { // TODO: move into a trait - pub fn text_input_state(&self) -> TextInputState { - unsafe { - let activity = (*self.as_ptr()).activity; - let mut out_state = TextInputState { - text: String::new(), - selection: TextSpan { start: 0, end: 0 }, - compose_region: None, - }; - let out_ptr = &mut out_state as *mut TextInputState; + /// Returns the current text input state + /// + /// If `take` is true then will check for some newly-flagged text input state and if set it will + /// clear the flag and return `Some` new state, otherwise it will return None. + /// + /// If `take` is false this this is guaranteed to return `Some` with the current text input + /// state. + pub fn text_input_state(&self, take: bool) -> Option { + self.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to get text input state after GameActivity was destroyed"); + return if take { + None + } else { + Some(TextInputState::default()) + }; + } + unsafe { + if take { + // XXX: The GameActivity implementation should be using + // atomic ops to set this flag, and require us to use + // atomics to check and clear it too. + // + // We currently just hope that with the lack of atomic ops that + // the compiler isn't reordering code so this gets flagged + // before the java main thread really updates the state. + if (*app_ptr).textInputState == 0 { + return None; + } + (*app_ptr).textInputState = 0; + } + let activity = (*app_ptr).activity; + let mut out_state = TextInputState { + text: String::new(), + selection: TextSpan { start: 0, end: 0 }, + compose_region: None, + }; + let out_ptr = &mut out_state as *mut TextInputState; - // NEON WARNING: - // - // It's not clearly documented but the GameActivity API over the - // GameTextInput library directly exposes _modified_ UTF8 text - // from Java so we need to be careful to convert text to and - // from UTF8 - // - // GameTextInput also uses a pre-allocated, fixed-sized buffer for - // the current text state and has shared `currentState_` that - // appears to have no lock to guard access from multiple threads. - // - // There's also no locking at the GameActivity level, so I'm fairly - // certain that `GameActivity_getTextInputState` isn't thread - // safe: https://issuetracker.google.com/issues/294112477 - // - // Overall this is all quite gnarly - and probably a good reminder - // of why we want to use Rust instead of C/C++. - ffi::GameActivity_getTextInputState( - activity, - Some(AndroidAppInner::map_input_state_to_text_event_callback), - out_ptr.cast(), - ); + // NEON WARNING: + // + // It's not clearly documented but the GameActivity API over the + // GameTextInput library directly exposes _modified_ UTF8 text + // from Java so we need to be careful to convert text to and + // from UTF8 + // + // GameTextInput also uses a pre-allocated, fixed-sized buffer for + // the current text state and has shared `currentState_` that + // appears to have no lock to guard access from multiple threads. + // + // There's also no locking at the GameActivity level, so I'm fairly + // certain that `GameActivity_getTextInputState` isn't thread + // safe: https://issuetracker.google.com/issues/294112477 + // + // Overall this is all quite gnarly - and probably a good reminder + // of why we want to use Rust instead of C/C++. + ffi::GameActivity_getTextInputState( + activity, + Some(AndroidAppInner::map_input_state_to_text_event_callback), + out_ptr.cast(), + ); - out_state - } + Some(out_state) + } + }) } - pub fn take_text_input_state(&self) -> TextInputState { - unsafe { - let app_ptr = self.as_ptr(); - (*app_ptr).textInputState = 0; - } - self.text_input_state() + pub fn take_text_input_state(&self) -> Option { + self.text_input_state(true) } pub fn set_ime_editor_info( @@ -213,85 +299,109 @@ impl NativeAppGlue { action: TextInputAction, options: ImeOptions, ) { - unsafe { - let activity = (*self.as_ptr()).activity; - let action_id: i32 = action.into(); + self.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to set IME editor info after GameActivity was destroyed"); + return; + } + unsafe { + let activity = (*app_ptr).activity; + let action_id: i32 = action.into(); - ffi::GameActivity_setImeEditorInfo( - activity, - input_type.bits(), - action_id as _, - options.bits(), - ); - } + ffi::GameActivity_setImeEditorInfo( + activity, + input_type.bits(), + action_id as _, + options.bits(), + ); + } + }); } // TODO: move into a trait pub fn set_text_input_state(&self, state: TextInputState) { - unsafe { - let activity = (*self.as_ptr()).activity; - let modified_utf8 = simd_cesu8::mutf8::encode(&state.text); - let text_length = modified_utf8.len() as i32; - let modified_utf8_bytes = modified_utf8.as_ptr(); - let ffi_state = ffi::GameTextInputState { - text_UTF8: modified_utf8_bytes.cast(), // NB: may be signed or unsigned depending on target - text_length, - selection: ffi::GameTextInputSpan { - start: state.selection.start as i32, - end: state.selection.end as i32, - }, - composingRegion: match state.compose_region { - Some(span) => { - // The GameText subclass of InputConnection only has a special case for removing the - // compose region if `start == -1` but the docs for `setComposingRegion` imply that - // the region should effectively be removed if any empty region is given (unlike for the - // selection region, it's not meaningful to maintain an empty compose region) - // - // We aim for more consistent behaviour by normalizing any empty region into `(-1, -1)` - // to remove the compose region. - // - // NB `setComposingRegion` itself is documented to clamp start/end to the text bounds - // so apart from this special-case handling in GameText's implementation of - // `setComposingRegion` then there's nothing special about `(-1, -1)` - it's just an empty - // region that should get clamped to `(0, 0)` and then get removed. - if span.start == span.end { - ffi::GameTextInputSpan { start: -1, end: -1 } - } else { - ffi::GameTextInputSpan { - start: span.start as i32, - end: span.end as i32, + self.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to set text input state after GameActivity was destroyed"); + return; + } + unsafe { + let activity = (*app_ptr).activity; + let modified_utf8 = simd_cesu8::mutf8::encode(&state.text); + let text_length = modified_utf8.len() as i32; + let modified_utf8_bytes = modified_utf8.as_ptr(); + let ffi_state = ffi::GameTextInputState { + text_UTF8: modified_utf8_bytes.cast(), // NB: may be signed or unsigned depending on target + text_length, + selection: ffi::GameTextInputSpan { + start: state.selection.start as i32, + end: state.selection.end as i32, + }, + composingRegion: match state.compose_region { + Some(span) => { + // The GameText subclass of InputConnection only has a special case for removing the + // compose region if `start == -1` but the docs for `setComposingRegion` imply that + // the region should effectively be removed if any empty region is given (unlike for the + // selection region, it's not meaningful to maintain an empty compose region) + // + // We aim for more consistent behaviour by normalizing any empty region into `(-1, -1)` + // to remove the compose region. + // + // NB `setComposingRegion` itself is documented to clamp start/end to the text bounds + // so apart from this special-case handling in GameText's implementation of + // `setComposingRegion` then there's nothing special about `(-1, -1)` - it's just an empty + // region that should get clamped to `(0, 0)` and then get removed. + if span.start == span.end { + ffi::GameTextInputSpan { start: -1, end: -1 } + } else { + ffi::GameTextInputSpan { + start: span.start as i32, + end: span.end as i32, + } } } - } - None => ffi::GameTextInputSpan { start: -1, end: -1 }, - }, - }; - ffi::GameActivity_setTextInputState(activity, &ffi_state as *const _); - } + None => ffi::GameTextInputSpan { start: -1, end: -1 }, + }, + }; + ffi::GameActivity_setTextInputState(activity, &ffi_state as *const _); + } + }) } pub fn take_pending_editor_action(&self) -> Option { - unsafe { - let app_ptr = self.as_ptr(); - if (*app_ptr).pendingEditorAction { - (*app_ptr).pendingEditorAction = false; - Some((*app_ptr).editorAction) - } else { - None + self.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!( + "Attempted to take pending editor action after GameActivity was destroyed" + ); + return None; } - } + unsafe { + if (*app_ptr).pendingEditorAction { + (*app_ptr).pendingEditorAction = false; + Some((*app_ptr).editorAction) + } else { + None + } + } + }) } } #[derive(Debug)] pub struct AndroidAppInner { pub(crate) jvm: jni::JavaVM, - native_app: NativeAppGlue, + game_activity: GameActivityGlue, config: ConfigurationRef, native_window: RwLock>, + activity: jni::refs::Global>, + pub(crate) main_callbacks: MainCallbacks, + /// Looper associated with the Rust `android_main` thread + looper: ndk::looper::ForeignLooper, + /// Looper associated with the activity's Java main thread, sometimes called /// the UI thread. main_looper: ndk::looper::ForeignLooper, @@ -317,8 +427,15 @@ pub struct AndroidAppInner { impl AndroidAppInner { pub fn activity_as_ptr(&self) -> *mut c_void { - let app_ptr = self.native_app.as_ptr(); - unsafe { (*(*app_ptr).activity).javaGameActivity as _ } + // Note: The global reference in `android_app` 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 { + self.looper.ptr().as_ptr() } pub fn native_window(&self) -> Option { @@ -336,10 +453,9 @@ impl AndroidAppInner { trace!("poll_events"); unsafe { - let native_app = &self.native_app; assert_eq!( ndk_sys::ALooper_forThread(), - (*native_app.as_ptr()).looper, + self.looper_as_ptr(), "Application tried to poll events from non-main thread" ); @@ -359,14 +475,23 @@ impl AndroidAppInner { &mut events, &mut source as *mut *mut core::ffi::c_void, ); + + // Always check to see if pollOnce woke up due to input being available + // (NB: we can't assume we will specifically get a POLL_WAKE event after a ALooper_wake()) + if self.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + false + } else { + ffi::android_app_input_available_wake_up(app_ptr) + } + }) { + log::debug!("Notifying Input Available"); + callback(PollEvent::Main(MainEvent::InputAvailable)); + } + match id { ffi::ALOOPER_POLL_WAKE => { trace!("ALooper_pollOnce returned POLL_WAKE"); - if ffi::android_app_input_available_wake_up(native_app.as_ptr()) { - log::debug!("Notifying Input Available"); - callback(PollEvent::Main(MainEvent::InputAvailable)); - } - callback(PollEvent::Wake); } ffi::ALOOPER_POLL_CALLBACK => { @@ -389,7 +514,7 @@ impl AndroidAppInner { trace!("ALooper_pollOnce returned ID_MAIN"); let source: *mut ffi::android_poll_source = source.cast(); if !source.is_null() { - let cmd_i = ffi::android_app_read_cmd(native_app.as_ptr()); + let cmd_i = ffi::android_app_read_cmd((*source).app); let cmd = match cmd_i as ffi::NativeAppGlueAppCmd { //NativeAppGlueAppCmd_UNUSED_APP_CMD_INPUT_CHANGED => AndroidAppMainEvent::InputChanged, @@ -456,19 +581,17 @@ impl AndroidAppInner { trace!("Read ID_MAIN command {cmd_i} = {cmd:?}"); trace!("Calling android_app_pre_exec_cmd({cmd_i})"); - ffi::android_app_pre_exec_cmd(native_app.as_ptr(), cmd_i); + ffi::android_app_pre_exec_cmd((*source).app, cmd_i); if let Some(cmd) = cmd { match cmd { MainEvent::ConfigChanged { .. } => { self.config.replace(Configuration::clone_from_ptr( - NonNull::new_unchecked( - (*native_app.as_ptr()).config, - ), + NonNull::new_unchecked((*(*source).app).config), )); } MainEvent::InitWindow { .. } => { - let win_ptr = (*native_app.as_ptr()).window; + let win_ptr = (*(*source).app).window; // It's important that we use ::clone_from_ptr() here // because NativeWindow has a Drop implementation that // will unconditionally _release() the native window @@ -477,18 +600,34 @@ impl AndroidAppInner { NonNull::new(win_ptr).unwrap(), )); } - MainEvent::TerminateWindow { .. } => { - *self.native_window.write().unwrap() = None; - } _ => {} } trace!("Invoking callback for ID_MAIN command = {:?}", cmd); callback(PollEvent::Main(cmd)); + + match cmd_i as ffi::NativeAppGlueAppCmd { + ffi::NativeAppGlueAppCmd_APP_CMD_TERM_WINDOW => { + *self.native_window.write().unwrap() = None; + } + ffi::NativeAppGlueAppCmd_APP_CMD_DESTROY => { + // We need to clear our `*mut android_app` pointer here because + // `android_native_app_glue.c` is going to free the `android_app` once it + // knows that this `android_main` thread has handled the `APP_CMD_DESTROY` + // event. In this case the Java main thread is in the middle + // of running `android_app_free()` in response to `onDestroy()`. + self.game_activity.clear_app(); + } + _ => {} + } } trace!("Calling android_app_post_exec_cmd({cmd_i})"); - ffi::android_app_post_exec_cmd(native_app.as_ptr(), cmd_i); + // SAFETY: Keep in mind that if we have just handled an `APP_CMD_DESTROY` event then we + // have just cleared our retained `android_app` pointer and the `(*source).app` pointer + // will become invalid after this call returns. In this case the Java main thread is in + // the middle of running `android_app_free()` in response to `onDestroy()`. + ffi::android_app_post_exec_cmd((*source).app, cmd_i); } else { panic!("ALooper_pollOnce returned ID_MAIN event with NULL android_poll_source!"); } @@ -510,36 +649,54 @@ impl AndroidAppInner { add_flags: WindowManagerFlags, remove_flags: WindowManagerFlags, ) { - unsafe { - let activity = (*self.native_app.as_ptr()).activity; - ffi::GameActivity_setWindowFlags(activity, add_flags.bits(), remove_flags.bits()) - } + self.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to set window flags after GameActivity was destroyed"); + return; + } + unsafe { + let activity = (*app_ptr).activity; + ffi::GameActivity_setWindowFlags(activity, add_flags.bits(), remove_flags.bits()) + } + }); } // TODO: move into a trait pub fn show_soft_input(&self, show_implicit: bool) { - unsafe { - let activity = (*self.native_app.as_ptr()).activity; - let flags = if show_implicit { - ffi::ShowImeFlags_SHOW_IMPLICIT - } else { - 0 - }; - ffi::GameActivity_showSoftInput(activity, flags); - } + self.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to show soft input after GameActivity was destroyed"); + return; + } + unsafe { + let activity = (*app_ptr).activity; + let flags = if show_implicit { + ffi::ShowImeFlags_SHOW_IMPLICIT + } else { + 0 + }; + ffi::GameActivity_showSoftInput(activity, flags); + } + }); } // TODO: move into a trait pub fn hide_soft_input(&self, hide_implicit_only: bool) { - unsafe { - let activity = (*self.native_app.as_ptr()).activity; - let flags = if hide_implicit_only { - ffi::HideImeFlags_HIDE_IMPLICIT_ONLY - } else { - 0 - }; - ffi::GameActivity_hideSoftInput(activity, flags); - } + self.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to hide soft input after GameActivity was destroyed"); + return; + } + unsafe { + let activity = (*app_ptr).activity; + let flags = if hide_implicit_only { + ffi::HideImeFlags_HIDE_IMPLICIT_ONLY + } else { + 0 + }; + ffi::GameActivity_hideSoftInput(activity, flags); + } + }); } unsafe extern "C" fn map_input_state_to_text_event_callback( @@ -579,12 +736,13 @@ impl AndroidAppInner { // TODO: move into a trait pub fn text_input_state(&self) -> TextInputState { - self.native_app.text_input_state() + // `.text_input_state` is guaranteed to return `Some` if `take` is `false` so we can unwrap here + self.game_activity.text_input_state(false).unwrap() } // TODO: move into a trait pub fn set_text_input_state(&self, state: TextInputState) { - self.native_app.set_text_input_state(state); + self.game_activity.set_text_input_state(state); } pub fn set_ime_editor_info( @@ -593,7 +751,7 @@ impl AndroidAppInner { action: TextInputAction, options: ImeOptions, ) { - self.native_app + self.game_activity .set_ime_editor_info(input_type, action, options); } @@ -626,11 +784,8 @@ impl AndroidAppInner { } pub fn create_waker(&self) -> AndroidAppWaker { - // Safety: we know that the app and looper pointers are valid - unsafe { - let app_ptr = self.native_app.as_ptr(); - AndroidAppWaker::new((*app_ptr).looper) - } + // Safety: we know that the looper is a valid, non-null pointer + unsafe { AndroidAppWaker::new(self.looper.ptr().as_ptr()) } } pub fn run_on_java_main_thread(&self, f: Box) @@ -645,15 +800,20 @@ impl AndroidAppInner { } pub fn content_rect(&self) -> Rect { - unsafe { - let app_ptr = self.native_app.as_ptr(); - Rect { - left: (*app_ptr).contentRect.left, - right: (*app_ptr).contentRect.right, - top: (*app_ptr).contentRect.top, - bottom: (*app_ptr).contentRect.bottom, + self.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to get content rect after GameActivity was destroyed"); + return Rect::default(); } - } + unsafe { + Rect { + left: (*app_ptr).contentRect.left, + right: (*app_ptr).contentRect.right, + top: (*app_ptr).contentRect.top, + bottom: (*app_ptr).contentRect.bottom, + } + } + }) } pub fn asset_manager(&self) -> AssetManager { @@ -679,7 +839,7 @@ impl AndroidAppInner { *guard = None; let receiver = Arc::new(InputReceiver { - native_app: self.native_app.clone(), + game_activity: self.game_activity.clone(), }); *guard = Some(Arc::downgrade(&receiver)); @@ -687,24 +847,33 @@ impl AndroidAppInner { } pub fn internal_data_path(&self) -> Option { - unsafe { - let app_ptr = self.native_app.as_ptr(); - try_get_path_from_ptr((*(*app_ptr).activity).internalDataPath) - } + self.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to get internal data path after GameActivity was destroyed"); + return None; + } + unsafe { try_get_path_from_ptr((*(*app_ptr).activity).internalDataPath) } + }) } pub fn external_data_path(&self) -> Option { - unsafe { - let app_ptr = self.native_app.as_ptr(); - try_get_path_from_ptr((*(*app_ptr).activity).externalDataPath) - } + self.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to get external data path after GameActivity was destroyed"); + return None; + } + unsafe { try_get_path_from_ptr((*(*app_ptr).activity).externalDataPath) } + }) } pub fn obb_path(&self) -> Option { - unsafe { - let app_ptr = self.native_app.as_ptr(); - try_get_path_from_ptr((*(*app_ptr).activity).obbPath) - } + self.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!("Attempted to get OBB path after GameActivity was destroyed"); + return None; + } + unsafe { try_get_path_from_ptr((*(*app_ptr).activity).obbPath) } + }) } } @@ -829,17 +998,26 @@ impl Drop for InputBuffer<'_> { /// API in any way while iterating events) #[derive(Debug)] pub(crate) struct InputReceiver { - // Safety: the native_app effectively has a static lifetime and it - // has its own internal locking when calling - // `android_app_swap_input_buffers` - native_app: NativeAppGlue, + // Safety: the `GameActivityGlue` effectively has a static lifetime and it + // has a mutex around the `*mut android_app` pointer to ensure we can't + // dereference a pointer that could be freed and `android_app` has its own + // internal locking when calling `android_app_swap_input_buffers` + game_activity: GameActivityGlue, } impl<'a> From> for InputIteratorInner<'a> { fn from(receiver: Arc) -> Self { let buffered = unsafe { - let app_ptr = receiver.native_app.as_ptr(); - let input_buffer = ffi::android_app_swap_input_buffers(app_ptr); + let input_buffer = receiver.game_activity.with_locked_app(|app_ptr| { + if app_ptr.is_null() { + log::error!( + "Attempting to swap input buffers after GameActivity was destroyed" + ); + // `null` here will result in `InputIteratorInner.buffered` being `None` below. + return ptr::null_mut(); + } + ffi::android_app_swap_input_buffers(app_ptr) + }); NonNull::new(input_buffer).map(|input_buffer| { let buffer = InputBuffer::from_ptr(input_buffer); let keys_iter = KeyEventsLendingIterator::new(&buffer); @@ -852,11 +1030,11 @@ impl<'a> From> for InputIteratorInner<'a> { }) }; - let native_app = receiver.native_app.clone(); + let game_activity = receiver.game_activity.clone(); Self { _receiver: receiver, buffered, - native_app, + game_activity, ime_text_input_state_checked: false, ime_editor_action_checked: false, } @@ -874,7 +1052,7 @@ pub(crate) struct InputIteratorInner<'a> { _receiver: Arc, buffered: Option>, - native_app: NativeAppGlue, + game_activity: GameActivityGlue, ime_text_input_state_checked: bool, ime_editor_action_checked: bool, } @@ -900,27 +1078,15 @@ impl InputIteratorInner<'_> { // for editor actions, so actions will apply to the latest state. if !self.ime_text_input_state_checked { self.ime_text_input_state_checked = true; - unsafe { - let app_ptr = self.native_app.as_ptr(); - - // XXX: It looks like the GameActivity implementation should - // be using atomic ops to set this flag, and require us to - // use atomics to check and clear it too. - // - // We currently just hope that with the lack of atomic ops that - // the compiler isn't reordering code so this gets flagged - // before the java main thread really updates the state. - if (*app_ptr).textInputState != 0 { - let state = self.native_app.take_text_input_state(); // Will clear .textInputState - let _ = callback(&InputEvent::TextEvent(state)); - return true; - } + if let Some(state) = self.game_activity.take_text_input_state() { + let _ = callback(&InputEvent::TextEvent(state)); + return true; } } if !self.ime_editor_action_checked { self.ime_editor_action_checked = true; - if let Some(action) = self.native_app.take_pending_editor_action() { + if let Some(action) = self.game_activity.take_pending_editor_action() { let _ = callback(&InputEvent::TextAction(TextInputAction::from(action))); return true; } @@ -993,7 +1159,7 @@ extern "Rust" { // This is called via `GameActivity.onCreate`, from the Java main/UI thread, // before spawning an `android_main` thread. #[no_mangle] -pub unsafe extern "C" fn _rust_glue_on_create_hook(_native_app: *mut ffi::android_app) { +pub unsafe extern "C" fn _rust_glue_on_create_hook(_game_activity_glue: *mut ffi::android_app) { // Noop currently } @@ -1001,20 +1167,20 @@ pub unsafe extern "C" fn _rust_glue_on_create_hook(_native_app: *mut ffi::androi // `android_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(native_app: *mut ffi::android_app) { +pub unsafe extern "C" fn _rust_glue_entry(game_activity_glue: *mut ffi::android_app) { abort_on_panic(|| { let _join_log_forwarder = forward_stdio_to_logcat(); let (jvm, jni_activity) = unsafe { - let jvm = (*(*native_app).activity).vm; - let activity: jobject = (*(*native_app).activity).javaGameActivity; + let jvm = (*(*game_activity_glue).activity).vm; + let activity: jobject = (*(*game_activity_glue).activity).javaGameActivity; (jni::JavaVM::from_raw(jvm), activity) }; // Note: At this point we can assume jni::JavaVM::singleton is initialized let main_looper = unsafe { ndk::looper::ForeignLooper::from_ptr( - std::ptr::NonNull::new((*native_app).mainLooper).unwrap(), + std::ptr::NonNull::new((*game_activity_glue).mainLooper).unwrap(), ) }; @@ -1026,6 +1192,8 @@ pub unsafe extern "C" fn _rust_glue_entry(native_app: *mut ffi::android_app) { // attachment, as a convenience. jvm.attach_current_thread(|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::>(&jni_activity)? }; let (app_asset_manager, main_callbacks) = @@ -1041,11 +1209,12 @@ pub unsafe extern "C" fn _rust_glue_entry(native_app: *mut ffi::android_app) { unsafe { let app = AndroidApp::new( - NonNull::new(native_app).unwrap(), jvm.clone(), - app_asset_manager, main_looper, main_callbacks, + app_asset_manager, + game_activity_glue, + &jni_activity, ); // We want to specifically catch any panic from the application's android_main // so we can finish + destroy the Activity gracefully via the JVM @@ -1066,7 +1235,7 @@ pub unsafe extern "C" fn _rust_glue_entry(native_app: *mut ffi::android_app) { // // "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); + ffi::GameActivity_finish((*game_activity_glue).activity); } Ok(()) diff --git a/android-activity/src/input.rs b/android-activity/src/input.rs index 2820b7f..b772a86 100644 --- a/android-activity/src/input.rs +++ b/android-activity/src/input.rs @@ -907,6 +907,16 @@ pub struct TextInputState { pub compose_region: Option, } +impl Default for TextInputState { + fn default() -> Self { + Self { + text: String::new(), + selection: TextSpan { start: 0, end: 0 }, + compose_region: None, + } + } +} + // Represents the action button on a soft keyboard. #[derive(Debug, Clone, Copy, PartialEq, Eq, num_enum::FromPrimitive, num_enum::IntoPrimitive)] #[non_exhaustive]