android: Hold NativeWindow lock until after notifying the user with Event::Suspended (#2307)

This applies https://github.com/rust-windowing/android-ndk-rs/issues/117
on the `winit` side: Android destroys its window/surface as soon as the
user returns from [`onNativeWindowDestroyed`], and we "fixed" this on
the `ndk-glue` side by sending the `WindowDestroyed` event before
locking the window and removing it: this lock has to wait for any user
of `ndk-glue` - ie. `winit` - to give up its readlock on the window,
which is what we utilize here to give users of `winit` "time" to destroy
any resource created on top of a `RawWindowHandle`.

since we can't pass the user a `RawWindowHandle` through the
`HasRawWindowHandle` trait we have to document this case explicitly and
keep the lock alive on the `winit` side instead.

[`onNativeWindowDestroyed`]: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks#onnativewindowdestroyed

CHANGELOG.md

@@ -8,6 +8,7 @@ And please only add new entries to the top of this list, right below the `# Unre
 
 # Unreleased
 
+- On Android, `ndk-glue`'s `NativeWindow` lock is now held between `Event::Resumed` and `Event::Suspended`.
 - On Web, added `EventLoopExtWebSys` with a `spawn` method to start the event loop without throwing an exception.
 - Added `WindowEvent::Occluded(bool)`, currently implemented on macOS and X11.
 - On X11, fix events for caps lock key not being sent

Cargo.toml

@@ -55,8 +55,8 @@ simple_logger = "2.1.0"
 
 [target.'cfg(target_os = "android")'.dependencies]
 # Coordinate the next winit release with android-ndk-rs: https://github.com/rust-windowing/winit/issues/1995
-ndk = { git = "https://github.com/rust-windowing/android-ndk-rs", rev = "7e33384" }
+ndk = { git = "https://github.com/rust-windowing/android-ndk-rs", rev = "a0c5e99" }
-ndk-glue = { git = "https://github.com/rust-windowing/android-ndk-rs", rev = "7e33384" }
+ndk-glue = { git = "https://github.com/rust-windowing/android-ndk-rs", rev = "a0c5e99" }
 
 [target.'cfg(any(target_os = "ios", target_os = "macos"))'.dependencies]
 objc = "0.2.7"

src/platform_impl/android/mod.rs

@@ -10,8 +10,9 @@ use ndk::{
     configuration::Configuration,
     event::{InputEvent, KeyAction, Keycode, MotionAction},
     looper::{ForeignLooper, Poll, ThreadLooper},
+    native_window::NativeWindow,
 };
-use ndk_glue::{Event, Rect};
+use ndk_glue::{Event, LockReadGuard, Rect};
 use once_cell::sync::Lazy;
 use raw_window_handle::{HasRawWindowHandle, RawWindowHandle};
 
@@ -239,6 +240,7 @@ pub struct EventLoop<T: 'static> {
     start_cause: event::StartCause,
     looper: ThreadLooper,
     running: bool,
+    window_lock: Option<LockReadGuard<NativeWindow>>,
 }
 
 #[derive(Default, Debug, Copy, Clone, PartialEq, Eq, Hash)]
@@ -268,6 +270,7 @@ impl<T: 'static> EventLoop<T> {
             start_cause: event::StartCause::Init,
             looper: ThreadLooper::for_thread().unwrap(),
             running: false,
+            window_lock: None,
         }
     }
 
@@ -300,22 +303,42 @@ impl<T: 'static> EventLoop<T> {
             match self.first_event.take() {
                 Some(EventSource::Callback) => match ndk_glue::poll_events().unwrap() {
                     Event::WindowCreated => {
-                        call_event_handler!(
+                        // Acquire a lock on the window to prevent Android from destroying
-                            event_handler,
+                        // it until we've notified and waited for the user in Event::Suspended.
-                            self.window_target(),
+                        // WARNING: ndk-glue is inherently racy (https://github.com/rust-windowing/winit/issues/2293)
-                            control_flow,
+                        // and may have already received onNativeWindowDestroyed while this thread hasn't yet processed
-                            event::Event::Resumed
+                        // the event, and would see a `None` lock+window in that case.
-                        );
+                        if let Some(next_window_lock) = ndk_glue::native_window() {
+                            assert!(
+                                self.window_lock.replace(next_window_lock).is_none(),
+                                "Received `Event::WindowCreated` while we were already holding a lock"
+                            );
+                            call_event_handler!(
+                                event_handler,
+                                self.window_target(),
+                                control_flow,
+                                event::Event::Resumed
+                            );
+                        } else {
+                            warn!("Received `Event::WindowCreated` while `ndk_glue::native_window()` provides no window");
+                        }
                     }
                     Event::WindowResized => resized = true,
                     Event::WindowRedrawNeeded => redraw = true,
                     Event::WindowDestroyed => {
-                        call_event_handler!(
+                        // Release the lock, allowing Android to clean up this surface
-                            event_handler,
+                        // WARNING: See above - if ndk-glue is racy, this event may be called
-                            self.window_target(),
+                        // without having a `self.window_lock` in place.
-                            control_flow,
+                        if self.window_lock.take().is_some() {
-                            event::Event::Suspended
+                            call_event_handler!(
-                        );
+                                event_handler,
+                                self.window_target(),
+                                control_flow,
+                                event::Event::Suspended
+                            );
+                        } else {
+                            warn!("Received `Event::WindowDestroyed` while we were not holding a window lock");
+                        }
                     }
                     Event::Pause => self.running = false,
                     Event::Resume => self.running = true,
@@ -369,7 +392,7 @@ impl<T: 'static> EventLoop<T> {
                 },
                 Some(EventSource::InputQueue) => {
                     if let Some(input_queue) = ndk_glue::input_queue().as_ref() {
-                        while let Some(event) = input_queue.get_event() {
+                        while let Some(event) = input_queue.get_event().expect("get_event") {
                             if let Some(event) = input_queue.pre_dispatch(event) {
                                 let mut handled = true;
                                 let window_id = window::WindowId(WindowId);
@@ -799,7 +822,7 @@ impl Window {
     }
 
     pub fn raw_window_handle(&self) -> RawWindowHandle {
-        if let Some(native_window) = ndk_glue::native_window().as_ref() {
+        if let Some(native_window) = ndk_glue::native_window() {
             native_window.raw_window_handle()
         } else {
             panic!("Cannot get the native window, it's null and will always be null before Event::Resumed and after Event::Suspended. Make sure you only call this function between those events.");

src/window.rs

@@ -1039,8 +1039,17 @@ unsafe impl raw_window_handle::HasRawWindowHandle for Window {
     ///
     /// ## Platform-specific
     ///
-    /// - **Android:** Only available after receiving the Resumed event and before Suspended. *If you*
+    /// ### Android
-    /// *try to get the handle outside of that period, this function will panic*!
+    ///
+    /// Only available after receiving [`Event::Resumed`] and before [`Event::Suspended`]. *If you
+    /// try to get the handle outside of that period, this function will panic*!
+    ///
+    /// Make sure to release or destroy any resources created from this `RawWindowHandle` (ie. Vulkan
+    /// or OpenGL surfaces) before returning from [`Event::Suspended`], at which point Android will
+    /// release the underlying window/surface: any subsequent interaction is undefined behavior.
+    ///
+    /// [`Event::Resumed`]: crate::event::Event::Resumed
+    /// [`Event::Suspended`]: crate::event::Event::Suspended
     fn raw_window_handle(&self) -> raw_window_handle::RawWindowHandle {
         self.window.raw_window_handle()
     }