Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hold on to Android NativeWindow lock to prevent races #1892

Closed
wants to merge 3 commits into from
Closed

Hold on to Android NativeWindow lock to prevent races #1892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c04011d
Hold on to Android NativeWindow lock to prevent races
maciejgodek Mar 20, 2021
cbeddb0
Convert comment into doc comment
maciejgodek Mar 24, 2021
b079b26
No longer obtain multiple read locks on Android NativeWindow
maciejgodek Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
85 changes: 61 additions & 24 deletions src/platform_impl/android/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ use ndk::{
configuration::Configuration,
event::{InputEvent, KeyAction, MotionAction},
looper::{ForeignLooper, Poll, ThreadLooper},
native_window::NativeWindow,
};
use ndk_glue::{Event, Rect};
use raw_window_handle::RawWindowHandle;
use std::{
collections::VecDeque,
sync::{Arc, Mutex, RwLock},
sync::{Arc, Mutex, RwLock, RwLockReadGuard},
time::{Duration, Instant},
};

Expand Down Expand Up @@ -43,6 +45,12 @@ fn poll(poll: Poll) -> Option<EventSource> {

pub struct EventLoop<T: 'static> {
window_target: event_loop::EventLoopWindowTarget<T>,
/// 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<RwLockReadGuard<'static, Option<NativeWindow>>>,
MarijnS95 marked this conversation as resolved.
Show resolved Hide resolved
user_queue: Arc<Mutex<VecDeque<T>>>,
first_event: Option<EventSource>,
start_cause: event::StartCause,
Expand All @@ -65,10 +73,12 @@ impl<T: 'static> EventLoop<T> {
Self {
window_target: event_loop::EventLoopWindowTarget {
p: EventLoopWindowTarget {
raw_window_handle: Default::default(),
_marker: std::marker::PhantomData,
},
_marker: std::marker::PhantomData,
},
native_window_lock: None,
user_queue: Default::default(),
first_event: None,
start_cause: event::StartCause::Init,
Expand Down Expand Up @@ -106,22 +116,36 @@ impl<T: 'static> EventLoop<T> {
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two calls remaining to ndk_glue::native_window(). IMO these should use self.native_window_lock and panic if it was None (implying it was called outside a Resumed - Suspended "block") instead of trying to grab the window on their own regard.

That might require some refactoring as fn raw_window_handle() only has access to struct Window which is currently empty, with EventLoopWindowTarget supposedly tying them together. I'm not sure what the desired flow is here, the other platform implementations should be able to help out here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried something like that first. My attempt ran into RwLockReadGuard being !Send and I gave that approach up. Will give it another try though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I came up with something. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have preferred to keep the entire thing in one place without two mutexes/locks. It looks like other backends like Wayland store modifyable data in EventLoopWindowTarget through a RefCell:

winit/src/platform_impl/linux/wayland/event_loop/mod.rs

Lines 50 to 51 in 86748fb

/// State that we share across callbacks.
pub state: RefCell<WinitState>,

That's where I'd put native_window_lock instead. Perhaps in a similar *State struct and/or with a type alias because it is getting quite long 😅

Maybe the winit-Android developers have a better overview of where to place (mutable) data shared between the EventLoop and Window; @msiglreith @dvc94ch?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that too, but I assumed that Window needs to be Send, which complicates things. I'll take another look at the other backends and see if I can glean anything more.

Copy link
Author

@maciejgodek maciejgodek Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Desktop platforms seem to make sure Window is Send and synchronize state with the event loop (?). It's not clear how important this is on Android, but if it is, then the fact that you can't put RwLockReadGuard behind an Arc<Mutex<T>> becomes a problem.

winit/src/platform_impl/linux/wayland/window/mod.rs

Line 46 in 86748fb

size: Arc<Mutex<LogicalSize<u32>>>,

winit/src/platform_impl/linux/x11/window.rs

Line 105 in 86748fb

pub shared_state: Mutex<SharedState>,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarijnS95 Still no idea how to solve this without extra synchronization. Some input from a maintainer might be useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I have not had the time to look at any alternatives either, seems like wrapping our state (either the readlock or raw window handle) inside yet another mutex is the only option - given Send limits this might only be possible with the window handle which has to be unconditionally retrieved and stored like this PR does currently.

It would have also been nice if the lock could be transposed to remove the Option (only want to hold the lock if it contains a valid window) but that is not possible either.

// 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,
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
);
}
self.window_target.p.update_native_window(None);
}
Event::Pause => self.running = false,
Event::Resume => self.running = true,
Expand Down Expand Up @@ -400,10 +424,19 @@ impl<T> Clone for EventLoopProxy<T> {
}

pub struct EventLoopWindowTarget<T: 'static> {
raw_window_handle: Arc<Mutex<Option<RawWindowHandle>>>,
_marker: std::marker::PhantomData<T>,
}

impl<T: 'static> EventLoopWindowTarget<T> {
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<monitor::MonitorHandle> {
Some(monitor::MonitorHandle {
inner: MonitorHandle,
Expand Down Expand Up @@ -438,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<Mutex<Option<RawWindowHandle>>>,
}

impl Window {
pub fn new<T: 'static>(
_el: &EventLoopWindowTarget<T>,
el: &EventLoopWindowTarget<T>,
_window_attrs: window::WindowAttributes,
_: PlatformSpecificWindowBuilderAttributes,
) -> Result<Self, error::OsError> {
// FIXME this ignores requested window attributes
Ok(Self)
Ok(Self {
raw_window_handle: Arc::clone(&el.raw_window_handle),
})
}

pub fn id(&self) -> WindowId {
Expand Down Expand Up @@ -563,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 {
Expand Down