From 6a73ad040b80adc7db42295f3f4641d511a1002a Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sat, 28 May 2022 21:04:10 +0200 Subject: [PATCH] android: Hold `NativeWindow` lock until after notifying the user with `Event::Suspended` 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 | 3 +- Cargo.toml | 4 +-- src/platform_impl/android/mod.rs | 53 +++++++++++++++++++++++--------- src/window.rs | 13 ++++++-- 4 files changed, 53 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a650ac24be..1cb080d3ff 100644 --- a/CHANGELOG.md +++ b/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 @@ -64,7 +65,7 @@ And please only add new entries to the top of this list, right below the `# Unre - Implemented `Eq` for `Fullscreen`, `Theme`, and `UserAttentionType`. - **Breaking:** `Window::set_cursor_grab` now accepts `CursorGrabMode` to control grabbing behavior. - On Wayland, add support for `Window::set_cursor_position`. -- Fix on macOS `WindowBuilder::with_disallow_hidpi`, setting true or false by the user no matter the SO default value. +- Fix on macOS `WindowBuilder::with_disallow_hidpi`, setting true or false by the user no matter the SO default value. - `EventLoopBuilder::build` will now panic when the `EventLoop` is being created more than once. - Added `From` for `WindowId` and `From` for `u64`. - Added `MonitorHandle::refresh_rate_millihertz` to get monitor's refresh rate. diff --git a/Cargo.toml b/Cargo.toml index ce8b35d109..403701127d 100644 --- a/Cargo.toml +++ b/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-glue = { 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 = "a0c5e99" } [target.'cfg(any(target_os = "ios", target_os = "macos"))'.dependencies] objc = "0.2.7" diff --git a/src/platform_impl/android/mod.rs b/src/platform_impl/android/mod.rs index 1d39c86233..31972b81c3 100644 --- a/src/platform_impl/android/mod.rs +++ b/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 { start_cause: event::StartCause, looper: ThreadLooper, running: bool, + window_lock: Option>, } #[derive(Default, Debug, Copy, Clone, PartialEq, Eq, Hash)] @@ -268,6 +270,7 @@ impl EventLoop { start_cause: event::StartCause::Init, looper: ThreadLooper::for_thread().unwrap(), running: false, + window_lock: None, } } @@ -300,22 +303,42 @@ impl EventLoop { match self.first_event.take() { Some(EventSource::Callback) => match ndk_glue::poll_events().unwrap() { Event::WindowCreated => { - call_event_handler!( - event_handler, - self.window_target(), - control_flow, - event::Event::Resumed - ); + // Acquire a lock on the window to prevent Android from destroying + // it until we've notified and waited for the user in Event::Suspended. + // WARNING: ndk-glue is inherently racy (https://github.com/rust-windowing/winit/issues/2293) + // and may have already received onNativeWindowDestroyed while this thread hasn't yet processed + // 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!( - event_handler, - self.window_target(), - control_flow, - event::Event::Suspended - ); + // Release the lock, allowing Android to clean up this surface + // WARNING: See above - if ndk-glue is racy, this event may be called + // without having a `self.window_lock` in place. + if self.window_lock.take().is_some() { + 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 EventLoop { }, 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."); diff --git a/src/window.rs b/src/window.rs index 2d732e8a49..50bc252618 100644 --- a/src/window.rs +++ b/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* - /// *try to get the handle outside of that period, this function will panic*! + /// ### Android + /// + /// 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() }