On Windows, catch window callback panics and forward them to the calling thread (#703)

* Catch windows callback panics

* Unwind through calling thread

* Add CHANGELOG entry

* Fix 1.24.1 builds

* Reformat CHANGELOG entry

* Make changes from review

* Wrap thread event target in panic catcher, reformat panic resume message

* Fix me being bad at git

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 - On X11, fixed panic caused by dropping the window before running the event loop.
 - Introduce `WindowBuilderExt::with_app_id` to allow setting the application ID on Wayland.
+- On Windows, catch panics in event loop child thread and forward them to the parent thread. This prevents an invocation of undefined behavior due to unwinding into foreign code.
 - On Windows, fix issue where resizing or moving window combined with grabbing the cursor would freeze program.
 - On Windows, fix issue where resizing or moving window would eat `Awakened` events.
 

Cargo.toml

@@ -35,6 +35,9 @@ cocoa = "0.18.4"
 core-foundation = "0.6"
 core-graphics = "0.17.3"
 
+[target.'cfg(target_os = "windows")'.dependencies]
+backtrace = "0.3"
+
 [target.'cfg(target_os = "windows")'.dependencies.winapi]
 version = "0.3.6"
 features = [

src/lib.rs

@@ -98,6 +98,8 @@ extern crate serde;
 
 #[cfg(target_os = "windows")]
 extern crate winapi;
+#[cfg(target_os = "windows")]
+extern crate backtrace;
 #[cfg(any(target_os = "macos", target_os = "ios"))]
 #[macro_use]
 extern crate objc;

src/platform/windows/events_loop.rs

@@ -12,12 +12,14 @@
 //! The closure passed to the `execute_in_thread` method takes an `Inserter` that you can use to
 //! add a `WindowState` entry to a list of window to be used by the callback.
 
-use std::{mem, ptr, thread};
+use std::{mem, panic, ptr, thread};
+use std::any::Any;
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::os::windows::io::AsRawHandle;
 use std::sync::{Arc, mpsc, Mutex};
 
+use backtrace::Backtrace;
 use winapi::ctypes::c_int;
 use winapi::shared::minwindef::{
     BOOL,
@@ -138,9 +140,14 @@ pub struct EventsLoop {
     // Id of the background thread from the Win32 API.
     thread_id: DWORD,
     // Receiver for the events. The sender is in the background thread.
-    receiver: mpsc::Receiver<Event>,
+    receiver: mpsc::Receiver<EventsLoopEvent>,
     // Sender instance that's paired with the receiver. Used to construct an `EventsLoopProxy`.
-    sender: mpsc::Sender<Event>,
+    sender: mpsc::Sender<EventsLoopEvent>,
+}
+
+enum EventsLoopEvent {
+    WinitEvent(Event),
+    Panic(PanicError),
 }
 
 impl EventsLoop {
@@ -164,6 +171,7 @@ impl EventsLoop {
         let (init_tx, init_rx) = mpsc::sync_channel(0);
 
         let thread_sender = tx.clone();
+        let panic_sender = tx.clone();
         let thread = thread::spawn(move || {
             let tx = thread_sender;
             let thread_msg_target = thread_event_target_window();
@@ -174,6 +182,7 @@ impl EventsLoop {
                     windows: HashMap::with_capacity(4),
                     file_drop_handlers: HashMap::with_capacity(4),
                     mouse_buttons_down: 0,
+                    panic_error: None,
                 });
             });
 
@@ -191,6 +200,16 @@ impl EventsLoop {
 
                 loop {
                     if winuser::GetMessageW(&mut msg, ptr::null_mut(), 0, 0) == 0 {
+                        // If a panic occurred in the child callback, forward the panic information
+                        // to the parent thread.
+                        let panic_payload_opt = CONTEXT_STASH.with(|stash|
+                            stash.borrow_mut().as_mut()
+                                 .and_then(|s| s.panic_error.take())
+                        );
+                        if let Some(panic_payload) = panic_payload_opt {
+                            panic_sender.send(EventsLoopEvent::Panic(panic_payload)).unwrap();
+                        };
+
                         // Only happens if the message is `WM_QUIT`.
                         debug_assert_eq!(msg.message, winuser::WM_QUIT);
                         break;
@@ -224,8 +243,12 @@ impl EventsLoop {
     {
         loop {
             let event = match self.receiver.try_recv() {
-                Ok(e) => e,
+                Ok(EventsLoopEvent::WinitEvent(e)) => e,
-                Err(_) => return
+                Ok(EventsLoopEvent::Panic(panic)) => {
+                    eprintln!("resuming child thread unwind at: {:?}", Backtrace::new());
+                    panic::resume_unwind(panic)
+                },
+                Err(_) => break,
             };
 
             callback(event);
@@ -237,8 +260,12 @@ impl EventsLoop {
     {
         loop {
             let event = match self.receiver.recv() {
-                Ok(e) => e,
+                Ok(EventsLoopEvent::WinitEvent(e)) => e,
-                Err(_) => return
+                Ok(EventsLoopEvent::Panic(panic)) => {
+                    eprintln!("resuming child thread unwind at: {:?}", Backtrace::new());
+                    panic::resume_unwind(panic)
+                },
+                Err(_) => break,
             };
 
             let flow = callback(event);
@@ -282,7 +309,7 @@ impl Drop for EventsLoop {
 #[derive(Clone)]
 pub struct EventsLoopProxy {
     thread_msg_target: HWND,
-    sender: mpsc::Sender<Event>,
+    sender: mpsc::Sender<EventsLoopEvent>,
 }
 
 unsafe impl Send for EventsLoopProxy {}
@@ -290,7 +317,7 @@ unsafe impl Sync for EventsLoopProxy {}
 
 impl EventsLoopProxy {
     pub fn wakeup(&self) -> Result<(), EventsLoopClosed> {
-        self.sender.send(Event::Awakened).map_err(|_| EventsLoopClosed)
+        self.sender.send(EventsLoopEvent::WinitEvent(Event::Awakened)).map_err(|_| EventsLoopClosed)
     }
 
     /// Executes a function in the background thread.
@@ -407,45 +434,78 @@ fn thread_event_target_window() -> HWND {
 // in a thread-local variable.
 thread_local!(static CONTEXT_STASH: RefCell<Option<ThreadLocalData>> = RefCell::new(None));
 struct ThreadLocalData {
-    sender: mpsc::Sender<Event>,
+    sender: mpsc::Sender<EventsLoopEvent>,
     windows: HashMap<HWND, Arc<Mutex<WindowState>>>,
     file_drop_handlers: HashMap<HWND, FileDropHandler>, // Each window has its own drop handler.
     mouse_buttons_down: u32,
+    panic_error: Option<PanicError>,
 }
+type PanicError = Box<Any + Send + 'static>;
 
 // Utility function that dispatches an event on the current thread.
 pub fn send_event(event: Event) {
     CONTEXT_STASH.with(|context_stash| {
         let context_stash = context_stash.borrow();
 
-        let _ = context_stash.as_ref().unwrap().sender.send(event);   // Ignoring if closed
+        let _ = context_stash.as_ref().unwrap().sender.send(EventsLoopEvent::WinitEvent(event));   // Ignoring if closed
     });
 }
 
 /// Capture mouse input, allowing `window` to receive mouse events when the cursor is outside of
 /// the window.
 unsafe fn capture_mouse(window: HWND) {
-    CONTEXT_STASH.with(|context_stash| {
+    let set_capture = CONTEXT_STASH.with(|context_stash| {
         let mut context_stash = context_stash.borrow_mut();
         if let Some(context_stash) = context_stash.as_mut() {
             context_stash.mouse_buttons_down += 1;
-            winuser::SetCapture(window);
+            true
+        } else {
+            false
         }
     });
+    if set_capture {
+        winuser::SetCapture(window);
+    }
 }
 
 /// Release mouse input, stopping windows on this thread from receiving mouse input when the cursor
 /// is outside the window.
 unsafe fn release_mouse() {
-    CONTEXT_STASH.with(|context_stash| {
+    let release_capture = CONTEXT_STASH.with(|context_stash| {
         let mut context_stash = context_stash.borrow_mut();
         if let Some(context_stash) = context_stash.as_mut() {
             context_stash.mouse_buttons_down = context_stash.mouse_buttons_down.saturating_sub(1);
             if context_stash.mouse_buttons_down == 0 {
-                winuser::ReleaseCapture();
+                return true;
             }
         }
+        false
     });
+    if release_capture {
+        winuser::ReleaseCapture();
+    }
+}
+
+pub unsafe fn run_catch_panic<F, R>(error: R, f: F) -> R
+    where F: panic::UnwindSafe + FnOnce() -> R
+{
+    // If a panic has been triggered, cancel all future operations in the function.
+    if CONTEXT_STASH.with(|stash| stash.borrow().as_ref().map(|s| s.panic_error.is_some()).unwrap_or(false)) {
+        return error;
+    }
+
+    let callback_result = panic::catch_unwind(f);
+    match callback_result {
+        Ok(lresult) => lresult,
+        Err(err) => CONTEXT_STASH.with(|context_stash| {
+            let mut context_stash = context_stash.borrow_mut();
+            if let Some(context_stash) = context_stash.as_mut() {
+                context_stash.panic_error = Some(err);
+                winuser::PostQuitMessage(-1);
+            }
+            error
+        })
+    }
 }
 
 /// Any window whose callback is configured to this function will have its events propagated
@@ -460,6 +520,17 @@ pub unsafe extern "system" fn callback(
     msg: UINT,
     wparam: WPARAM,
     lparam: LPARAM,
+) -> LRESULT {
+    // Unwinding into foreign code is undefined behavior. So we catch any panics that occur in our
+    // code, and if a panic happens we cancel any future operations.
+    run_catch_panic(-1, || callback_inner(window, msg, wparam, lparam))
+}
+
+unsafe fn callback_inner(
+    window: HWND,
+    msg: UINT,
+    wparam: WPARAM,
+    lparam: LPARAM,
 ) -> LRESULT {
     match msg {
         winuser::WM_CREATE => {
@@ -564,7 +635,7 @@ pub unsafe extern "system" fn callback(
                     event: Resized(logical_size),
                 };
 
-                cstash.sender.send(event).ok();
+                cstash.sender.send(EventsLoopEvent::WinitEvent(event)).ok();
             });
             0
         },
@@ -1214,12 +1285,15 @@ pub unsafe extern "system" fn thread_event_target_callback(
     wparam: WPARAM,
     lparam: LPARAM,
 ) -> LRESULT {
-    match msg {
+    // See `callback` comment.
-        _ if msg == *EXEC_MSG_ID => {
+    run_catch_panic(-1, || {
-            let mut function: Box<Box<FnMut()>> = Box::from_raw(wparam as usize as *mut _);
+        match msg {
-            function();
+            _ if msg == *EXEC_MSG_ID => {
-            0
+                let mut function: Box<Box<FnMut()>> = Box::from_raw(wparam as usize as *mut _);
-        },
+                function();
-        _ => winuser::DefWindowProcW(window, msg, wparam, lparam)
+                0
-    }
+            },
+            _ => winuser::DefWindowProcW(window, msg, wparam, lparam)
+        }
+    })
 }