From 28e3c35547609ac40a472f656f51e54882a793b7 Mon Sep 17 00:00:00 2001 From: Osspial Date: Fri, 18 Oct 2019 11:51:06 -0400 Subject: [PATCH] Prevent EventLoop from getting initialized outside the main thread in cross-platform functions (#1186) * Prevent EventLoop from getting initialized outside the main thread This only applies to the cross-platform functions. We expose functions to do this in a platform-specific manner, when available. * Add CHANGELOG entry * Formatting has changed since the latest stable update... * Fix error spacing * Unix: Prevent initializing EventLoop outside main thread * Updates libc dependency to 0.2.64, as required by BSD platforms * Update CHANGELOG.md for Linux implementation * Finish sentence * Consolidate documentation --- CHANGELOG.md | 4 ++ Cargo.toml | 2 +- src/event_loop.rs | 18 ++++-- src/platform/unix.rs | 79 +++++++++++++++++++++---- src/platform/windows.rs | 36 ++++++++++- src/platform_impl/linux/mod.rs | 60 +++++++++++++++++-- src/platform_impl/windows/dpi.rs | 5 +- src/platform_impl/windows/event_loop.rs | 55 ++++++++++++++--- 8 files changed, 224 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c27607e5..d1f12a52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,10 @@ - On X11, fix use-after-free during window creation - On Windows, disable monitor change keyboard shortcut while in exclusive fullscreen. - On Windows, ensure that changing a borderless fullscreen window's monitor via keyboard shortcuts keeps the window fullscreen on the new monitor. +- Prevent `EventLoop::new` and `EventLoop::with_user_event` from getting called outside the main thread. + - This is because some platforms cannot run the event loop outside the main thread. Preventing this + reduces the potential for cross-platform compatibility gotchyas. +- On Windows and Linux X11/Wayland, add platform-specific functions for creating an `EventLoop` outside the main thread. # 0.20.0 Alpha 3 (2019-08-14) diff --git a/Cargo.toml b/Cargo.toml index 679d9065..8a9ec520 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ stdweb = ["std_web", "instant/stdweb"] [dependencies] instant = "0.1" lazy_static = "1" -libc = "0.2" +libc = "0.2.64" log = "0.4" serde = { version = "1", optional = true, features = ["serde_derive"] } raw-window-handle = "0.3" diff --git a/src/event_loop.rs b/src/event_loop.rs index 5bb9692b..0935fc72 100644 --- a/src/event_loop.rs +++ b/src/event_loop.rs @@ -28,6 +28,7 @@ use crate::{event::Event, monitor::MonitorHandle, platform_impl}; /// forbidding it), as such it is neither `Send` nor `Sync`. If you need cross-thread access, the /// `Window` created from this `EventLoop` _can_ be sent to an other thread, and the /// `EventLoopProxy` allows you to wake up an `EventLoop` from another thread. +/// pub struct EventLoop { pub(crate) event_loop: platform_impl::EventLoop, pub(crate) _marker: ::std::marker::PhantomData<*mut ()>, // Not Send nor Sync @@ -94,6 +95,18 @@ impl Default for ControlFlow { impl EventLoop<()> { /// Builds a new event loop with a `()` as the user event type. /// + /// ***For cross-platform compatibility, the `EventLoop` must be created on the main thread.*** + /// Attempting to create the event loop on a different thread will panic. This restriction isn't + /// strictly necessary on all platforms, but is imposed to eliminate any nasty surprises when + /// porting to platforms that require it. `EventLoopExt::new_any_thread` functions are exposed + /// in the relevant `platform` module if the target platform supports creating an event loop on + /// any thread. + /// + /// Usage will result in display backend initialisation, this can be controlled on linux + /// using an environment variable `WINIT_UNIX_BACKEND`. Legal values are `x11` and `wayland`. + /// If it is not set, winit will try to connect to a wayland connection, and if it fails will + /// fallback on x11. If this variable is set with any other value, winit will panic. + /// /// ## Platform-specific /// /// - **iOS:** Can only be called on the main thread. @@ -105,10 +118,7 @@ impl EventLoop<()> { impl EventLoop { /// Builds a new event loop. /// - /// Usage will result in display backend initialisation, this can be controlled on linux - /// using an environment variable `WINIT_UNIX_BACKEND`. Legal values are `x11` and `wayland`. - /// If it is not set, winit will try to connect to a wayland connection, and if it fails will - /// fallback on x11. If this variable is set with any other value, winit will panic. + /// All caveats documented in [`EventLoop::new`] apply to this function. /// /// ## Platform-specific /// diff --git a/src/platform/unix.rs b/src/platform/unix.rs index a72b1d33..61921e1d 100644 --- a/src/platform/unix.rs +++ b/src/platform/unix.rs @@ -144,35 +144,90 @@ impl EventLoopWindowTargetExtUnix for EventLoopWindowTarget { /// Additional methods on `EventLoop` that are specific to Unix. pub trait EventLoopExtUnix { - /// Builds a new `EventLoops` that is forced to use X11. + /// Builds a new `EventLoop` that is forced to use X11. + /// + /// # Panics + /// + /// If called outside the main thread. To initialize an X11 event loop outside + /// the main thread, use [`new_x11_any_thread`](#tymethod.new_x11_any_thread). fn new_x11() -> Result where Self: Sized; /// Builds a new `EventLoop` that is forced to use Wayland. + /// + /// # Panics + /// + /// If called outside the main thread. To initialize a Wayland event loop outside + /// the main thread, use [`new_wayland_any_thread`](#tymethod.new_wayland_any_thread). fn new_wayland() -> Self where Self: Sized; + + /// Builds a new `EventLoop` on any thread. + /// + /// This method bypasses the cross-platform compatibility requirement + /// that `EventLoop` be created on the main thread. + fn new_any_thread() -> Self + where + Self: Sized; + + /// Builds a new X11 `EventLoop` on any thread. + /// + /// This method bypasses the cross-platform compatibility requirement + /// that `EventLoop` be created on the main thread. + fn new_x11_any_thread() -> Result + where + Self: Sized; + + /// Builds a new Wayland `EventLoop` on any thread. + /// + /// This method bypasses the cross-platform compatibility requirement + /// that `EventLoop` be created on the main thread. + fn new_wayland_any_thread() -> Self + where + Self: Sized; +} + +fn wrap_ev(event_loop: LinuxEventLoop) -> EventLoop { + EventLoop { + event_loop, + _marker: std::marker::PhantomData, + } } impl EventLoopExtUnix for EventLoop { + #[inline] + fn new_any_thread() -> Self { + wrap_ev(LinuxEventLoop::new_any_thread()) + } + + #[inline] + fn new_x11_any_thread() -> Result { + LinuxEventLoop::new_x11_any_thread().map(wrap_ev) + } + + #[inline] + fn new_wayland_any_thread() -> Self { + wrap_ev( + LinuxEventLoop::new_wayland_any_thread() + // TODO: propagate + .expect("failed to open Wayland connection"), + ) + } + #[inline] fn new_x11() -> Result { - LinuxEventLoop::new_x11().map(|ev| EventLoop { - event_loop: ev, - _marker: ::std::marker::PhantomData, - }) + LinuxEventLoop::new_x11().map(wrap_ev) } #[inline] fn new_wayland() -> Self { - EventLoop { - event_loop: match LinuxEventLoop::new_wayland() { - Ok(e) => e, - Err(_) => panic!(), // TODO: propagate - }, - _marker: ::std::marker::PhantomData, - } + wrap_ev( + LinuxEventLoop::new_wayland() + // TODO: propagate + .expect("failed to open Wayland connection"), + ) } } diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 0b93b330..3ab9b1dd 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -15,18 +15,52 @@ use crate::{ /// Additional methods on `EventLoop` that are specific to Windows. pub trait EventLoopExtWindows { + /// Creates an event loop off of the main thread. + /// + /// # `Window` caveats + /// + /// Note that any `Window` created on the new thread will be destroyed when the thread + /// terminates. Attempting to use a `Window` after its parent thread terminates has + /// unspecified, although explicitly not undefined, behavior. + fn new_any_thread() -> Self + where + Self: Sized; + /// By default, winit on Windows will attempt to enable process-wide DPI awareness. If that's /// undesirable, you can create an `EventLoop` using this function instead. fn new_dpi_unaware() -> Self where Self: Sized; + + /// Creates a DPI-unaware event loop off of the main thread. + /// + /// The `Window` caveats in [`new_any_thread`](EventLoopExtWindows::new_any_thread) also apply here. + fn new_dpi_unaware_any_thread() -> Self + where + Self: Sized; } impl EventLoopExtWindows for EventLoop { + #[inline] + fn new_any_thread() -> Self { + EventLoop { + event_loop: WindowsEventLoop::new_any_thread(), + _marker: ::std::marker::PhantomData, + } + } + #[inline] fn new_dpi_unaware() -> Self { EventLoop { - event_loop: WindowsEventLoop::with_dpi_awareness(false), + event_loop: WindowsEventLoop::new_dpi_unaware(), + _marker: ::std::marker::PhantomData, + } + } + + #[inline] + fn new_dpi_unaware_any_thread() -> Self { + EventLoop { + event_loop: WindowsEventLoop::new_dpi_unaware_any_thread(), _marker: ::std::marker::PhantomData, } } diff --git a/src/platform_impl/linux/mod.rs b/src/platform_impl/linux/mod.rs index 7b71de28..3e56b824 100644 --- a/src/platform_impl/linux/mod.rs +++ b/src/platform_impl/linux/mod.rs @@ -520,14 +520,22 @@ impl Clone for EventLoopProxy { impl EventLoop { pub fn new() -> EventLoop { + assert_is_main_thread("new_any_thread"); + + EventLoop::new_any_thread() + } + + pub fn new_any_thread() -> EventLoop { if let Ok(env_var) = env::var(BACKEND_PREFERENCE_ENV_VAR) { match env_var.as_str() { "x11" => { // TODO: propagate - return EventLoop::new_x11().expect("Failed to initialize X11 backend"); + return EventLoop::new_x11_any_thread() + .expect("Failed to initialize X11 backend"); } "wayland" => { - return EventLoop::new_wayland().expect("Failed to initialize Wayland backend"); + return EventLoop::new_wayland_any_thread() + .expect("Failed to initialize Wayland backend"); } _ => panic!( "Unknown environment variable value for {}, try one of `x11`,`wayland`", @@ -536,12 +544,12 @@ impl EventLoop { } } - let wayland_err = match EventLoop::new_wayland() { + let wayland_err = match EventLoop::new_wayland_any_thread() { Ok(event_loop) => return event_loop, Err(err) => err, }; - let x11_err = match EventLoop::new_x11() { + let x11_err = match EventLoop::new_x11_any_thread() { Ok(event_loop) => return event_loop, Err(err) => err, }; @@ -554,10 +562,22 @@ impl EventLoop { } pub fn new_wayland() -> Result, ConnectError> { + assert_is_main_thread("new_wayland_any_thread"); + + EventLoop::new_wayland_any_thread() + } + + pub fn new_wayland_any_thread() -> Result, ConnectError> { wayland::EventLoop::new().map(EventLoop::Wayland) } pub fn new_x11() -> Result, XNotSupported> { + assert_is_main_thread("new_x11_any_thread"); + + EventLoop::new_x11_any_thread() + } + + pub fn new_x11_any_thread() -> Result, XNotSupported> { X11_BACKEND .lock() .as_ref() @@ -672,3 +692,35 @@ fn sticky_exit_callback( // user callback callback(evt, target, cf) } + +fn assert_is_main_thread(suggested_method: &str) { + if !is_main_thread() { + panic!( + "Initializing the event loop outside of the main thread is a significant \ + cross-platform compatibility hazard. If you really, absolutely need to create an \ + EventLoop on a different thread, please use the `EventLoopExtUnix::{}` function.", + suggested_method + ); + } +} + +#[cfg(target_os = "linux")] +fn is_main_thread() -> bool { + use libc::{c_long, getpid, syscall, SYS_gettid}; + + unsafe { syscall(SYS_gettid) == getpid() as c_long } +} + +#[cfg(any(target_os = "dragonfly", target_os = "freebsd", target_os = "openbsd"))] +fn is_main_thread() -> bool { + use libc::pthread_main_np; + + unsafe { pthread_main_np() == 1 } +} + +#[cfg(target_os = "netbsd")] +fn is_main_thread() -> bool { + use libc::_lwp_self; + + unsafe { _lwp_self() == 1 } +} diff --git a/src/platform_impl/windows/dpi.rs b/src/platform_impl/windows/dpi.rs index 1a5fa13c..05af503e 100644 --- a/src/platform_impl/windows/dpi.rs +++ b/src/platform_impl/windows/dpi.rs @@ -43,10 +43,7 @@ lazy_static! { get_function!("user32.dll", EnableNonClientDpiScaling); } -pub fn become_dpi_aware(enable: bool) { - if !enable { - return; - } +pub fn become_dpi_aware() { static ENABLE_DPI_AWARENESS: Once = Once::new(); ENABLE_DPI_AWARENESS.call_once(|| { unsafe { diff --git a/src/platform_impl/windows/event_loop.rs b/src/platform_impl/windows/event_loop.rs index b9a0846a..20c1dd06 100644 --- a/src/platform_impl/windows/event_loop.rs +++ b/src/platform_impl/windows/event_loop.rs @@ -132,18 +132,40 @@ pub struct EventLoopWindowTarget { pub(crate) runner_shared: EventLoopRunnerShared, } +macro_rules! main_thread_check { + ($fn_name:literal) => {{ + let thread_id = unsafe { processthreadsapi::GetCurrentThreadId() }; + if thread_id != main_thread_id() { + panic!(concat!( + "Initializing the event loop outside of the main thread is a significant \ + cross-platform compatibility hazard. If you really, absolutely need to create an \ + EventLoop on a different thread, please use the `EventLoopExtWindows::", + $fn_name, + "` function." + )); + } + }}; +} + impl EventLoop { pub fn new() -> EventLoop { - Self::with_dpi_awareness(true) + main_thread_check!("new_any_thread"); + + Self::new_any_thread() } - pub fn window_target(&self) -> &RootELW { - &self.window_target + pub fn new_any_thread() -> EventLoop { + become_dpi_aware(); + Self::new_dpi_unaware_any_thread() } - pub fn with_dpi_awareness(dpi_aware: bool) -> EventLoop { - become_dpi_aware(dpi_aware); + pub fn new_dpi_unaware() -> EventLoop { + main_thread_check!("new_dpi_unaware_any_thread"); + Self::new_dpi_unaware_any_thread() + } + + pub fn new_dpi_unaware_any_thread() -> EventLoop { let thread_id = unsafe { processthreadsapi::GetCurrentThreadId() }; let runner_shared = Rc::new(ELRShared { runner: RefCell::new(None), @@ -166,6 +188,10 @@ impl EventLoop { } } + pub fn window_target(&self) -> &RootELW { + &self.window_target + } + pub fn run(mut self, event_handler: F) -> ! where F: 'static + FnMut(Event, &RootELW, &mut ControlFlow), @@ -178,10 +204,6 @@ impl EventLoop { where F: FnMut(Event, &RootELW, &mut ControlFlow), { - unsafe { - winuser::IsGUIThread(1); - } - let event_loop_windows_ref = &self.window_target; let mut runner = unsafe { @@ -280,6 +302,21 @@ impl EventLoopWindowTarget { } } +fn main_thread_id() -> DWORD { + static mut MAIN_THREAD_ID: DWORD = 0; + #[used] + #[allow(non_upper_case_globals)] + #[link_section = ".CRT$XCU"] + static INIT_MAIN_THREAD_ID: unsafe fn() = { + unsafe fn initer() { + MAIN_THREAD_ID = processthreadsapi::GetCurrentThreadId(); + } + initer + }; + + unsafe { MAIN_THREAD_ID } +} + pub(crate) type EventLoopRunnerShared = Rc>; pub(crate) struct ELRShared { runner: RefCell>>,