From c04011daa5a30fe308265c391e0360732f265118 Mon Sep 17 00:00:00 2001 From: Maciej Godek Date: Sun, 21 Mar 2021 00:01:12 +0100 Subject: [PATCH 1/3] Hold on to Android NativeWindow lock to prevent races In order to ensure validity of the raw pointers to Android's `ANativeWindow` handed out by `Window::raw_window_handle()`, the `EventLoop` will hold on to a read lock on the `NativeWindow` returned by `ndk_glue::native_window()` between receiving the `WindowCreated` event and the matching `WindowDestroyed` event from `ndk-glue`'s event pipe. Note that this commit depends on recent fixes to `ndk-glue`, specifically this PR https://github.com/rust-windowing/android-ndk-rs/pull/134. Previous versions of `ndk-glue` will cause this code to deadlock due to a concurrency bug. --- Cargo.toml | 6 ++--- src/platform_impl/android/mod.rs | 42 ++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1af7d4d23e..0e22b4d19a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,9 +37,9 @@ image = "0.23.12" simple_logger = "1.9" [target.'cfg(target_os = "android")'.dependencies] -ndk = "0.3" -ndk-sys = "0.2.0" -ndk-glue = "0.3" +ndk = { path = "../android-ndk-rs/ndk" } +ndk-sys = { path = "../android-ndk-rs/ndk-sys" } +ndk-glue = { path = "../android-ndk-rs/ndk-glue" } [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 543bca1b03..898beebcc0 100644 --- a/src/platform_impl/android/mod.rs +++ b/src/platform_impl/android/mod.rs @@ -10,11 +10,12 @@ use ndk::{ configuration::Configuration, event::{InputEvent, KeyAction, MotionAction}, looper::{ForeignLooper, Poll, ThreadLooper}, + native_window::NativeWindow, }; use ndk_glue::{Event, Rect}; use std::{ collections::VecDeque, - sync::{Arc, Mutex, RwLock}, + sync::{Arc, Mutex, RwLock, RwLockReadGuard}, time::{Duration, Instant}, }; @@ -43,6 +44,10 @@ fn poll(poll: Poll) -> Option { pub struct EventLoop { window_target: event_loop::EventLoopWindowTarget, + // This read guard will be held between each pair of `Resumed` and `Suspended` + // events to ensure that `ndk_glue` does not release the `NativeWindow` prematurely, + // invalidating raw handles obtained by the user through `Window::raw_window_handle()` + native_window_lock: Option>>, user_queue: Arc>>, first_event: Option, start_cause: event::StartCause, @@ -69,6 +74,7 @@ impl EventLoop { }, _marker: std::marker::PhantomData, }, + native_window_lock: None, user_queue: Default::default(), first_event: None, start_cause: event::StartCause::Init, @@ -106,22 +112,32 @@ 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 - ); + let native_window_lock = ndk_glue::native_window(); + // The window could have gone away before we got the message + if native_window_lock.is_some() { + self.native_window_lock = Some(native_window_lock); + call_event_handler!( + event_handler, + self.window_target(), + control_flow, + event::Event::Resumed + ); + } } Event::WindowResized => resized = true, Event::WindowRedrawNeeded => redraw = true, Event::WindowDestroyed => { - call_event_handler!( - event_handler, - self.window_target(), - control_flow, - event::Event::Suspended - ); + let native_window_lock = self.native_window_lock.take(); + // This event is ignored if no window was actually grabbed onto + // in `WindowCreated` above + if native_window_lock.is_some() { + call_event_handler!( + event_handler, + self.window_target(), + control_flow, + event::Event::Suspended + ); + } } Event::Pause => self.running = false, Event::Resume => self.running = true, From cbeddb00dc3f4ec6b3742a26e1b1e4ba84b4dd96 Mon Sep 17 00:00:00 2001 From: maciejgodek Date: Wed, 24 Mar 2021 10:03:44 +0100 Subject: [PATCH 2/3] Convert comment into doc comment Co-authored-by: Marijn Suijten --- src/platform_impl/android/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/platform_impl/android/mod.rs b/src/platform_impl/android/mod.rs index 898beebcc0..df2b303358 100644 --- a/src/platform_impl/android/mod.rs +++ b/src/platform_impl/android/mod.rs @@ -44,9 +44,11 @@ fn poll(poll: Poll) -> Option { pub struct EventLoop { window_target: event_loop::EventLoopWindowTarget, - // This read guard will be held between each pair of `Resumed` and `Suspended` - // events to ensure that `ndk_glue` does not release the `NativeWindow` prematurely, - // invalidating raw handles obtained by the user through `Window::raw_window_handle()` + /// This read guard will be held between each pair of + /// [`event::Event::Resumed`] and [`event::Event::Suspended`] events to + /// ensure that [`ndk_glue`] does not release the [`NativeWindow`] + /// prematurely, invalidating raw handles obtained by the user through + /// [`Window::raw_window_handle()`]. native_window_lock: Option>>, user_queue: Arc>>, first_event: Option, From b079b260423904aee6d914ca2a8347de26da0867 Mon Sep 17 00:00:00 2001 From: Maciej Godek Date: Wed, 24 Mar 2021 11:23:53 +0100 Subject: [PATCH 3/3] No longer obtain multiple read locks on Android NativeWindow --- src/platform_impl/android/mod.rs | 41 +++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/src/platform_impl/android/mod.rs b/src/platform_impl/android/mod.rs index df2b303358..72570766cc 100644 --- a/src/platform_impl/android/mod.rs +++ b/src/platform_impl/android/mod.rs @@ -13,6 +13,7 @@ use ndk::{ native_window::NativeWindow, }; use ndk_glue::{Event, Rect}; +use raw_window_handle::RawWindowHandle; use std::{ collections::VecDeque, sync::{Arc, Mutex, RwLock, RwLockReadGuard}, @@ -72,6 +73,7 @@ impl EventLoop { Self { window_target: event_loop::EventLoopWindowTarget { p: EventLoopWindowTarget { + raw_window_handle: Default::default(), _marker: std::marker::PhantomData, }, _marker: std::marker::PhantomData, @@ -117,6 +119,9 @@ impl EventLoop { let native_window_lock = ndk_glue::native_window(); // The window could have gone away before we got the message if native_window_lock.is_some() { + self.window_target + .p + .update_native_window(native_window_lock.as_ref()); self.native_window_lock = Some(native_window_lock); call_event_handler!( event_handler, @@ -140,6 +145,7 @@ impl EventLoop { event::Event::Suspended ); } + self.window_target.p.update_native_window(None); } Event::Pause => self.running = false, Event::Resume => self.running = true, @@ -418,10 +424,19 @@ impl Clone for EventLoopProxy { } pub struct EventLoopWindowTarget { + raw_window_handle: Arc>>, _marker: std::marker::PhantomData, } impl EventLoopWindowTarget { + fn update_native_window(&self, native_window: Option<&NativeWindow>) { + *self.raw_window_handle.lock().unwrap() = native_window.map(|native_window| { + let mut handle = raw_window_handle::android::AndroidHandle::empty(); + handle.a_native_window = unsafe { native_window.ptr().as_mut() as *mut _ as *mut _ }; + RawWindowHandle::Android(handle) + }); + } + pub fn primary_monitor(&self) -> Option { Some(monitor::MonitorHandle { inner: MonitorHandle, @@ -456,16 +471,20 @@ impl DeviceId { #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] pub struct PlatformSpecificWindowBuilderAttributes; -pub struct Window; +pub struct Window { + raw_window_handle: Arc>>, +} impl Window { pub fn new( - _el: &EventLoopWindowTarget, + el: &EventLoopWindowTarget, _window_attrs: window::WindowAttributes, _: PlatformSpecificWindowBuilderAttributes, ) -> Result { // FIXME this ignores requested window attributes - Ok(Self) + Ok(Self { + raw_window_handle: Arc::clone(&el.raw_window_handle), + }) } pub fn id(&self) -> WindowId { @@ -581,14 +600,14 @@ impl Window { } pub fn raw_window_handle(&self) -> raw_window_handle::RawWindowHandle { - let a_native_window = if let Some(native_window) = ndk_glue::native_window().as_ref() { - unsafe { native_window.ptr().as_mut() as *mut _ as *mut _ } - } 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."); - }; - let mut handle = raw_window_handle::android::AndroidHandle::empty(); - handle.a_native_window = a_native_window; - raw_window_handle::RawWindowHandle::Android(handle) + self.raw_window_handle + .lock() + .unwrap() + .as_ref() + .expect( + "The window can be obtained only between `Event::Resumed` and `Event::Suspended`!", + ) + .clone() } pub fn config(&self) -> Configuration {