From 28adee1326c47cb6798edde6e199086c003822e0 Mon Sep 17 00:00:00 2001 From: Robert Bragg Date: Tue, 18 Apr 2023 23:54:47 +0100 Subject: [PATCH] windows: ensure pump_events returns after RedrawRequested This ensures pump_events can't block the external loop due to a flood of input events This also removes a boat-load of `unsafe` designations for internal functions which don't seem to do anything unsafe (and they also have no `# Safety` docs that suggest that there's anything unsafe about them) --- src/platform_impl/android/mod.rs | 3 + src/platform_impl/windows/event_loop.rs | 221 ++++++++++++------ .../windows/event_loop/runner.rs | 49 ++-- 3 files changed, 182 insertions(+), 91 deletions(-) diff --git a/src/platform_impl/android/mod.rs b/src/platform_impl/android/mod.rs index 6e1bc1705e..3d6fa612a5 100644 --- a/src/platform_impl/android/mod.rs +++ b/src/platform_impl/android/mod.rs @@ -196,6 +196,9 @@ fn ndk_keycode_to_virtualkeycode(keycode: Keycode) -> Option`, taking into account that `None` +/// equates to an infinite timeout, not a zero timeout (so can't just use +/// `Option::min`) fn min_timeout(a: Option, b: Option) -> Option { a.map_or(b, |a_timeout| { b.map_or(Some(a_timeout), |b_timeout| Some(a_timeout.min(b_timeout))) diff --git a/src/platform_impl/windows/event_loop.rs b/src/platform_impl/windows/event_loop.rs index 60b100e2bc..f74e574cd9 100644 --- a/src/platform_impl/windows/event_loop.rs +++ b/src/platform_impl/windows/event_loop.rs @@ -54,7 +54,7 @@ use windows_sys::Win32::{ GetMenu, GetMessageW, KillTimer, LoadCursorW, PeekMessageW, PostMessageW, RegisterClassExW, RegisterWindowMessageA, SetCursor, SetTimer, SetWindowPos, TranslateMessage, CREATESTRUCTW, GIDC_ARRIVAL, GIDC_REMOVAL, GWL_STYLE, GWL_USERDATA, - HTCAPTION, HTCLIENT, MINMAXINFO, MNC_CLOSE, NCCALCSIZE_PARAMS, PM_REMOVE, PT_PEN, + HTCAPTION, HTCLIENT, MINMAXINFO, MNC_CLOSE, MSG, NCCALCSIZE_PARAMS, PM_REMOVE, PT_PEN, PT_TOUCH, RI_KEY_E0, RI_KEY_E1, RI_MOUSE_WHEEL, SC_MINIMIZE, SC_RESTORE, SIZE_MAXIMIZED, SWP_NOACTIVATE, SWP_NOMOVE, SWP_NOSIZE, SWP_NOZORDER, WHEEL_DELTA, WINDOWPOS, WM_CAPTURECHANGED, WM_CHAR, WM_CLOSE, WM_CREATE, WM_DESTROY, WM_DPICHANGED, @@ -254,6 +254,9 @@ impl EventLoop { } let event_loop_windows_ref = &self.window_target; + // # Safety + // We make sure to call runner.clear_event_handler() before + // returning unsafe { runner.set_event_handler(move |event, control_flow| { event_handler(event, event_loop_windows_ref, control_flow) @@ -261,23 +264,22 @@ impl EventLoop { } } - let exit_code = unsafe { - 'main: loop { - if let ControlFlow::ExitWithCode(code) = self.wait_and_dispatch_message() { - break 'main code; - } + let exit_code = loop { + if let ControlFlow::ExitWithCode(code) = self.wait_and_dispatch_message() { + break code; + } - if let ControlFlow::ExitWithCode(code) = self.dispatch_peeked_messages() { - break 'main code; - } + if let ControlFlow::ExitWithCode(code) = self.dispatch_peeked_messages() { + break code; } }; let runner = &self.window_target.p.runner_shared; - unsafe { - runner.loop_destroyed(); - } + runner.loop_destroyed(); + // # Safety + // We assume that this will effectively call `runner.clear_event_handler()` + // to meet the safety requirements for calling `runner.set_event_handler()` above. runner.reset_runner(); if exit_code == 0 { @@ -294,6 +296,14 @@ impl EventLoop { { let runner = &self.window_target.p.runner_shared; let event_loop_windows_ref = &self.window_target; + + // # Safety + // We make sure to call runner.clear_event_handler() before + // returning + // + // Note: we're currently assuming nothing can panic and unwind + // to leave the runner in an unsound state with an associated + // event handler. unsafe { runner.set_event_handler(move |event, control_flow| { event_handler(event, event_loop_windows_ref, control_flow) @@ -302,120 +312,173 @@ impl EventLoop { } } - unsafe { - self.dispatch_peeked_messages(); - }; + self.dispatch_peeked_messages(); let runner = &self.window_target.p.runner_shared; let status = if let ControlFlow::ExitWithCode(code) = runner.control_flow() { - unsafe { - runner.loop_destroyed(); + runner.loop_destroyed(); - // Immediately reset the internal state for the loop to allow - // the loop to be run more than once. - runner.reset_runner(); - } + // Immediately reset the internal state for the loop to allow + // the loop to be run more than once. + runner.reset_runner(); PumpStatus::Exit(code) } else { - unsafe { - runner.prepare_wait(); - } + runner.prepare_wait(); PumpStatus::Continue }; - // Need to wait until we've checked for an exit status, in case we need to dispatch - // a LoopDestroyed event + // We wait until we've checked for an exit status before clearing the + // application callback, in case we need to dispatch a LoopDestroyed event + // + // # Safety + // This pairs up with our call to `runner.set_event_handler` and ensures + // the application's callback can't be held beyond its lifetime. runner.clear_event_handler(); status } /// Wait for one message and dispatch it, optionally with a timeout if control_flow is `WaitUntil` - unsafe fn wait_and_dispatch_message(&mut self) -> ControlFlow { - let mut msg = mem::zeroed(); + fn wait_and_dispatch_message(&mut self) -> ControlFlow { + let start = Instant::now(); let runner = &self.window_target.p.runner_shared; - // We aim to be consistent with the MacOS backend which has a RunLoop - // observer that will dispatch MainEventsCleared when about to wait for - // events, and NewEvents after the RunLoop wakes up. - // - // We emulate similar behaviour by treating `GetMessage` as our wait - // point and wake up point (when it returns) and we drain all other - // pending messages via `PeekMessage` until we come back to "wait" via - // `GetMessage` - // - runner.prepare_wait(); - - let start = Instant::now(); - let timeout = match runner.control_flow() { ControlFlow::Wait => None, - ControlFlow::Poll => Some(Duration::from_millis(0)), + ControlFlow::Poll => Some(Duration::ZERO), ControlFlow::WaitUntil(wait_deadline) => { if wait_deadline > start { Some(wait_deadline - start) } else { - Some(Duration::from_millis(0)) + Some(Duration::ZERO) } } ControlFlow::ExitWithCode(_code) => unreachable!(), }; - if let Some(timeout) = timeout { - // XXX: how should we pick a timer ID? - SetTimer(0, 0xf00, dur2timeout(timeout), None); - } else { - KillTimer(0, 0xf00); + + fn get_msg_with_timeout(msg: &mut MSG, timeout: Option) -> PumpStatus { + unsafe { + // A timeout of None means wait indefinitely (so we don't need to call SetTimer) + let timer_id = timeout.map(|timeout| SetTimer(0, 0, dur2timeout(timeout), None)); + let get_status = GetMessageW(msg, 0, 0, 0); + if let Some(timer_id) = timer_id { + KillTimer(0, timer_id); + } + // A return value of 0 implies `WM_QUIT` + if get_status == 0 { + PumpStatus::Exit(0) + } else { + PumpStatus::Continue + } + } } - if GetMessageW(&mut msg, 0, 0, 0) == false.into() { - // A return value of 0 implies `WM_QUIT` - runner.set_exit_control_flow(); - return runner.control_flow(); + /// Fetch the next MSG either via PeekMessage or GetMessage depending on whether the + /// requested timeout is `ZERO` (and so we don't want to block) + /// + /// Returns `None` if if no MSG was read, else a `Continue` or `Exit` status + fn wait_for_msg(msg: &mut MSG, timeout: Option) -> Option { + if timeout == Some(Duration::ZERO) { + unsafe { + if PeekMessageW(msg, 0, 0, 0, PM_REMOVE) != 0 { + Some(PumpStatus::Continue) + } else { + None + } + } + } else { + Some(get_msg_with_timeout(msg, timeout)) + } } + // We aim to be consistent with the MacOS backend which has a RunLoop + // observer that will dispatch MainEventsCleared when about to wait for + // events, and NewEvents after the RunLoop wakes up. + // + // We emulate similar behaviour by treating `GetMessage` as our wait + // point and wake up point (when it returns) and we drain all other + // pending messages via `PeekMessage` until we come back to "wait" via + // `GetMessage` + // + runner.prepare_wait(); + + // # Safety + // The Windows API has no documented requirement for bitwise + // initializing a `MSG` struct (it can be uninitialized memory for the C + // API) and there's no API to construct or initialize a `MSG`. This + // is the simplest way avoid unitialized memory in Rust + let mut msg = unsafe { mem::zeroed() }; + let msg_status = wait_for_msg(&mut msg, timeout); + + // Before we potentially exit, make sure to consistently emit an event for the wake up runner.wakeup(); - let handled = if let Some(callback) = self.msg_hook.as_deref_mut() { - callback(&mut msg as *mut _ as *mut _) - } else { - false - }; - if !handled { - TranslateMessage(&msg); - DispatchMessageW(&msg); - } + match msg_status { + None => {} // No MSG to dispatch + Some(PumpStatus::Exit(code)) => { + runner.set_exit_control_flow(code); + return runner.control_flow(); + } + Some(PumpStatus::Continue) => { + unsafe { + let handled = if let Some(callback) = self.msg_hook.as_deref_mut() { + callback(&mut msg as *mut _ as *mut _) + } else { + false + }; + if !handled { + TranslateMessage(&msg); + DispatchMessageW(&msg); + } + } - if let Err(payload) = runner.take_panic_error() { - runner.reset_runner(); - panic::resume_unwind(payload); + if let Err(payload) = runner.take_panic_error() { + runner.reset_runner(); + panic::resume_unwind(payload); + } + } } runner.control_flow() } /// Dispatch all queued messages via `PeekMessageW` - unsafe fn dispatch_peeked_messages(&mut self) -> ControlFlow { + fn dispatch_peeked_messages(&mut self) -> ControlFlow { let runner = &self.window_target.p.runner_shared; - let mut msg = mem::zeroed(); + // We generally want to continue dispatching all pending messages + // but we also allow dispatching to be interrupted as a means to + // ensure the `pump_events` won't indefinitely block an external + // event loop if there are too many pending events. This interrupt + // flag will be set after dispatching `RedrawRequested` events. + runner.interrupt_msg_dispatch.set(false); + + // # Safety + // The Windows API has no documented requirement for bitwise + // initializing a `MSG` struct (it can be uninitialized memory for the C + // API) and there's no API to construct or initialize a `MSG`. This + // is the simplest way avoid unitialized memory in Rust + let mut msg = unsafe { mem::zeroed() }; let mut control_flow = runner.control_flow(); loop { - if PeekMessageW(&mut msg, 0, 0, 0, PM_REMOVE) == false.into() { - break; - } + unsafe { + if PeekMessageW(&mut msg, 0, 0, 0, PM_REMOVE) == false.into() { + break; + } - let handled = if let Some(callback) = self.msg_hook.as_deref_mut() { - callback(&mut msg as *mut _ as *mut _) - } else { - false - }; - if !handled { - TranslateMessage(&msg); - DispatchMessageW(&msg); + let handled = if let Some(callback) = self.msg_hook.as_deref_mut() { + callback(&mut msg as *mut _ as *mut _) + } else { + false + }; + if !handled { + TranslateMessage(&msg); + DispatchMessageW(&msg); + } } if let Err(payload) = runner.take_panic_error() { @@ -427,6 +490,10 @@ impl EventLoop { if let ControlFlow::ExitWithCode(_code) = control_flow { break; } + + if runner.interrupt_msg_dispatch.get() { + break; + } } control_flow diff --git a/src/platform_impl/windows/event_loop/runner.rs b/src/platform_impl/windows/event_loop/runner.rs index 5646f4029d..c39b3f7004 100644 --- a/src/platform_impl/windows/event_loop/runner.rs +++ b/src/platform_impl/windows/event_loop/runner.rs @@ -28,6 +28,11 @@ pub(crate) struct EventLoopRunner { // The event loop's win32 handles pub(super) thread_msg_target: HWND, + // Setting this will ensure pump_events will return to the external + // loop asap. E.g. set after each RedrawRequested to ensure pump_events + // can't stall an external loop beyond a frame + pub(super) interrupt_msg_dispatch: Cell, + control_flow: Cell, runner_state: Cell, last_events_cleared: Cell, @@ -49,9 +54,6 @@ pub(crate) enum RunnerState { /// The event loop is handling the OS's events and sending them to the user's callback. /// `NewEvents` has been sent, and `MainEventsCleared` hasn't. HandlingMainEvents, - /// The event loop is handling the redraw events and sending them to the user's callback. - /// `MainEventsCleared` has been sent, and `RedrawEventsCleared` hasn't. - //HandlingRedrawEvents, /// The event loop has been destroyed. No other events will be emitted. Destroyed, } @@ -65,6 +67,7 @@ impl EventLoopRunner { pub(crate) fn new(thread_msg_target: HWND) -> EventLoopRunner { EventLoopRunner { thread_msg_target, + interrupt_msg_dispatch: Cell::new(false), runner_state: Cell::new(RunnerState::Uninitialized), control_flow: Cell::new(ControlFlow::Poll), panic_error: Cell::new(None), @@ -74,6 +77,17 @@ impl EventLoopRunner { } } + /// Associate the application's event handler with the runner + /// + /// # Safety + /// This is ignoring the lifetime of the application handler (which may not + /// outlive the EventLoopRunner) and can lead to undefined behaviour if + /// the handler is not cleared before the end of real lifetime. + /// + /// All public APIs that take an event handler (`run`, `run_ondemand`, + /// `pump_events`) _must_ pair a call to `set_event_handler` with + /// a call to `clear_event_handler` before returning to avoid + /// undefined behaviour. pub(crate) unsafe fn set_event_handler(&self, f: F) where F: FnMut(Event<'_, T>, &mut ControlFlow), @@ -92,6 +106,7 @@ impl EventLoopRunner { pub(crate) fn reset_runner(&self) { let EventLoopRunner { thread_msg_target: _, + interrupt_msg_dispatch, runner_state, panic_error, control_flow, @@ -99,6 +114,7 @@ impl EventLoopRunner { event_handler, event_buffer: _, } = self; + interrupt_msg_dispatch.set(false); runner_state.set(RunnerState::Uninitialized); panic_error.set(None); control_flow.set(ControlFlow::Poll); @@ -108,6 +124,7 @@ impl EventLoopRunner { /// State retrieval functions. impl EventLoopRunner { + #[allow(unused)] pub fn thread_msg_target(&self) -> HWND { self.thread_msg_target } @@ -123,8 +140,8 @@ impl EventLoopRunner { self.runner_state.get() } - pub fn set_exit_control_flow(&self) { - self.control_flow.set(ControlFlow::ExitWithCode(0)) + pub fn set_exit_control_flow(&self, code: i32) { + self.control_flow.set(ControlFlow::ExitWithCode(code)) } pub fn control_flow(&self) -> ControlFlow { @@ -171,17 +188,21 @@ impl EventLoopRunner { /// Event dispatch functions. impl EventLoopRunner { - pub(crate) unsafe fn prepare_wait(&self) { + pub(crate) fn prepare_wait(&self) { self.move_state_to(RunnerState::Idle); } - pub(crate) unsafe fn wakeup(&self) { + pub(crate) fn wakeup(&self) { self.move_state_to(RunnerState::HandlingMainEvents); } - pub(crate) unsafe fn send_event(&self, event: Event<'_, T>) { + pub(crate) fn send_event(&self, event: Event<'_, T>) { if let Event::RedrawRequested(_) = event { self.call_event_handler(event); + // As a rule, to ensure that `pump_events` can't block an external event loop + // for too long, we always guarantee that `pump_events` will return control to + // the external loop asap after a `RedrawRequested` event is dispatched. + self.interrupt_msg_dispatch.set(true); } else if self.should_buffer() { // If the runner is already borrowed, we're in the middle of an event loop invocation. Add // the event to a buffer to be processed later. @@ -194,11 +215,11 @@ impl EventLoopRunner { } } - pub(crate) unsafe fn loop_destroyed(&self) { + pub(crate) fn loop_destroyed(&self) { self.move_state_to(RunnerState::Destroyed); } - unsafe fn call_event_handler(&self, event: Event<'_, T>) { + fn call_event_handler(&self, event: Event<'_, T>) { self.catch_unwind(|| { let mut control_flow = self.control_flow.take(); let mut event_handler = self.event_handler.take() @@ -215,7 +236,7 @@ impl EventLoopRunner { }); } - unsafe fn dispatch_buffered_events(&self) { + fn dispatch_buffered_events(&self) { loop { // We do this instead of using a `while let` loop because if we use a `while let` // loop the reference returned `borrow_mut()` doesn't get dropped until the end @@ -252,7 +273,7 @@ impl EventLoopRunner { /// state is a no-op. Even if the `new_runner_state` isn't the immediate next state in the /// runner state machine (e.g. `self.runner_state == HandlingMainEvents` and /// `new_runner_state == Idle`), the intermediate state transitions will still be executed. - unsafe fn move_state_to(&self, new_runner_state: RunnerState) { + fn move_state_to(&self, new_runner_state: RunnerState) { use RunnerState::{Destroyed, HandlingMainEvents, Idle, Uninitialized}; match ( @@ -303,7 +324,7 @@ impl EventLoopRunner { } } - unsafe fn call_new_events(&self, init: bool) { + fn call_new_events(&self, init: bool) { let start_cause = match (init, self.control_flow()) { (true, _) => StartCause::Init, (false, ControlFlow::Poll) => StartCause::Poll, @@ -336,7 +357,7 @@ impl EventLoopRunner { self.dispatch_buffered_events(); } - unsafe fn call_redraw_events_cleared(&self) { + fn call_redraw_events_cleared(&self) { self.call_event_handler(Event::RedrawEventsCleared); self.last_events_cleared.set(Instant::now()); }