Fix unsoundness on Windows (#601)

* Fix unsoundness in windows backend

* Synchronize window state properly

* update changelog and add a comment to execute_in_thread

* Formatting fixes and improve changelog message

CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased
 
+- Fixed thread-safety issues in several `Window` functions on Windows. 
 - On MacOS, the key state for modifiers key events is now properly set.
 - On iOS, the view is now set correctly. This makes it possible to render things (instead of being stuck on a black screen), and touch events work again.
 - Added NetBSD support.

src/platform/windows/events_loop.rs

@@ -58,6 +58,7 @@ use platform::platform::dpi::{
     get_hwnd_scale_factor,
 };
 use platform::platform::event::{handle_extended_keys, process_key_params, vkey_to_winit_vkey};
+use platform::platform::icon::WinIcon;
 use platform::platform::raw_input::{get_raw_input_data, get_raw_mouse_button_state};
 use platform::platform::window::adjust_size;
 
@@ -94,6 +95,13 @@ pub struct WindowState {
     // This is different from the value in `SavedWindowInfo`! That one represents the DPI saved upon entering
     // fullscreen. This will always be the most recent DPI for the window.
     pub dpi_factor: f64,
+    pub fullscreen: Option<::MonitorId>,
+    pub window_icon: Option<WinIcon>,
+    pub taskbar_icon: Option<WinIcon>,
+    pub decorations: bool,
+    pub always_on_top: bool,
+    pub maximized: bool,
+    pub resizable: bool,
 }
 
 impl WindowState {
@@ -326,6 +334,11 @@ impl EventsLoopProxy {
     ///
     /// The `Inserted` can be used to inject a `WindowState` for the callback to use. The state is
     /// removed automatically if the callback receives a `WM_CLOSE` message for the window.
+    ///
+    /// Note that if you are using this to change some property of a window and updating
+    /// `WindowState` then you should call this within the lock of `WindowState`. Otherwise the
+    /// events may be sent to the other thread in different order to the one in which you set
+    /// `WindowState`, leaving them out of sync.
     pub fn execute_in_thread<F>(&self, function: F)
     where
         F: FnMut(Inserter) + Send + 'static,

src/platform/windows/icon.rs

@@ -22,11 +22,13 @@ pub enum IconType {
     Big = winuser::ICON_BIG as isize,
 }
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub struct WinIcon {
     pub handle: HICON,
 }
 
+unsafe impl Send for WinIcon {}
+
 impl WinIcon {
     #[allow(dead_code)]
     pub fn from_path<P: AsRef<Path>>(path: P) -> Result<Self, util::WinError> {

src/platform/windows/window.rs

@@ -1,8 +1,8 @@
 #![cfg(target_os = "windows")]
 
-use std::cell::{Cell, RefCell};
-use std::ffi::OsStr;
 use std::{io, mem, ptr};
+use std::cell::Cell;
+use std::ffi::OsStr;
 use std::os::windows::ffi::OsStrExt;
 use std::sync::{Arc, Mutex};
 use std::sync::mpsc::channel;
@@ -11,7 +11,7 @@ use winapi::ctypes::c_int;
 use winapi::shared::minwindef::{BOOL, DWORD, FALSE, LPARAM, TRUE, UINT, WORD, WPARAM};
 use winapi::shared::windef::{HDC, HWND, LPPOINT, POINT, RECT};
 use winapi::um::{combaseapi, dwmapi, libloaderapi, winuser};
-use winapi::um::objbase::{COINIT_MULTITHREADED};
+use winapi::um::objbase::COINIT_MULTITHREADED;
 use winapi::um::shobjidl_core::{CLSID_TaskbarList, ITaskbarList2};
 use winapi::um::winnt::{LONG, LPCWSTR};
 
@@ -27,7 +27,8 @@ use {
 };
 use platform::platform::{Cursor, PlatformSpecificWindowBuilderAttributes, WindowId};
 use platform::platform::dpi::{dpi_to_scale_factor, get_window_dpi, get_window_scale_factor};
-use platform::platform::events_loop::{self, DESTROY_MSG_ID, EventsLoop, INITIAL_DPI_MSG_ID};
+use platform::platform::events_loop::{self, EventsLoop, DESTROY_MSG_ID, INITIAL_DPI_MSG_ID};
+use platform::platform::events_loop::WindowState;
 use platform::platform::icon::{self, IconType, WinIcon};
 use platform::platform::monitor::get_available_monitors;
 use platform::platform::raw_input::register_all_mice_and_keyboards_for_raw_input;
@@ -40,25 +41,13 @@ pub struct Window {
     /// Main handle for the window.
     window: WindowWrapper,
 
-    decorations: Cell<bool>,
-    maximized: Cell<bool>,
-    resizable: Cell<bool>,
-    fullscreen: RefCell<Option<::MonitorId>>,
-    always_on_top: Cell<bool>,
-
     /// The current window state.
-    window_state: Arc<Mutex<events_loop::WindowState>>,
+    window_state: Arc<Mutex<WindowState>>,
-
-    window_icon: Cell<Option<WinIcon>>,
-    taskbar_icon: Cell<Option<WinIcon>>,
 
     // The events loop proxy.
     events_loop_proxy: events_loop::EventsLoopProxy,
 }
 
-unsafe impl Send for Window {}
-unsafe impl Sync for Window {}
-
 // https://blogs.msdn.microsoft.com/oldnewthing/20131017-00/?p=2903
 // The idea here is that we use the Adjust­Window­Rect­Ex function to calculate how much additional
 // non-client area gets added due to the styles we passed. To make the math simple,
@@ -283,32 +272,25 @@ impl Window {
 
     #[inline]
     pub fn set_resizable(&self, resizable: bool) {
-        if resizable == self.resizable.get() {
+        let mut window_state = self.window_state.lock().unwrap();
-            return;
+        if mem::replace(&mut window_state.resizable, resizable) != resizable {
-        }
-        if self.fullscreen.borrow().is_some() {
             // If we're in fullscreen, update stored configuration but don't apply anything.
-            self.resizable.replace(resizable);
+            if window_state.fullscreen.is_none() {
-            return;
+                let mut style = unsafe { 
-        }
+                    winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE) 
+                };
 
-        let mut style = unsafe {
+                if resizable {
-            winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE)
+                    style |= WS_RESIZABLE as LONG;
-        };
+                } else {
-        if resizable {
+                    style &= !WS_RESIZABLE as LONG;
-            style |= WS_RESIZABLE as LONG;
+                }
-        } else {
-            style &= !WS_RESIZABLE as LONG;
-        }
 
-        unsafe {
+                unsafe {
-            winuser::SetWindowLongW(
+                    winuser::SetWindowLongW(self.window.0, winuser::GWL_STYLE, style as _);
-                self.window.0,
+                };
-                winuser::GWL_STYLE,
+            }
-                style as _,
+        }
-            );
-        };
-        self.resizable.replace(resizable);
     }
 
     /// Returns the `hwnd` of this window.
@@ -391,15 +373,12 @@ impl Window {
     #[inline]
     pub fn grab_cursor(&self, grab: bool) -> Result<(), String> {
         let currently_grabbed = unsafe { self.cursor_is_grabbed() }?;
-        let window_state = Arc::clone(&self.window_state);
+        let window_state_lock = self.window_state.lock().unwrap();
-        {
+        if currently_grabbed == grab && grab == window_state_lock.cursor_grabbed {
-            let window_state_lock = window_state.lock().unwrap();
+            return Ok(());
-            if currently_grabbed == grab
-            && grab == window_state_lock.cursor_grabbed {
-                return Ok(());
-            }
         }
         let window = self.window.clone();
+        let window_state = Arc::clone(&self.window_state);
         let (tx, rx) = channel();
         self.events_loop_proxy.execute_in_thread(move |_| {
             let result = unsafe { Self::grab_cursor_inner(&window, grab) };
@@ -408,6 +387,7 @@ impl Window {
             }
             let _ = tx.send(result);
         });
+        drop(window_state_lock);
         rx.recv().unwrap()
     }
 
@@ -421,18 +401,17 @@ impl Window {
 
     #[inline]
     pub fn hide_cursor(&self, hide: bool) {
-        let window_state = Arc::clone(&self.window_state);
+        let window_state_lock = self.window_state.lock().unwrap();
-        {
+        // We don't want to increment/decrement the display count more than once!
-            let window_state_lock = window_state.lock().unwrap();
+        if hide == window_state_lock.cursor_hidden { return; }
-            // We don't want to increment/decrement the display count more than once!
-            if hide == window_state_lock.cursor_hidden { return; }
-        }
         let (tx, rx) = channel();
+        let window_state = Arc::clone(&self.window_state);
         self.events_loop_proxy.execute_in_thread(move |_| {
             unsafe { Self::hide_cursor_inner(hide) };
             window_state.lock().unwrap().cursor_hidden = hide;
             let _ = tx.send(());
         });
+        drop(window_state_lock);
         rx.recv().unwrap()
     }
 
@@ -468,9 +447,12 @@ impl Window {
 
     #[inline]
     pub fn set_maximized(&self, maximized: bool) {
-        self.maximized.replace(maximized);
+        let mut window_state = self.window_state.lock().unwrap();
+        window_state.maximized = true;
         // We only maximize if we're not in fullscreen.
-        if self.fullscreen.borrow().is_some() { return; }
+        if window_state.fullscreen.is_none() {
+            return;
+        }
 
         let window = self.window.clone();
         unsafe {
@@ -488,10 +470,8 @@ impl Window {
         }
     }
 
-    unsafe fn set_fullscreen_style(&self) -> (LONG, LONG) {
+    unsafe fn set_fullscreen_style(&self, window_state: &mut WindowState) -> (LONG, LONG) {
-        let mut window_state = self.window_state.lock().unwrap();
+        if window_state.fullscreen.is_none() || window_state.saved_window_info.is_none() {
-
-        if self.fullscreen.borrow().is_none() || window_state.saved_window_info.is_none() {
             let rect = util::get_window_rect(self.window.0).expect("`GetWindowRect` failed");
             let dpi_factor = Some(self.get_hidpi_factor());
             window_state.saved_window_info = Some(events_loop::SavedWindowInfo {
@@ -507,16 +487,14 @@ impl Window {
         let mut placement: winuser::WINDOWPLACEMENT = mem::zeroed();
         placement.length = mem::size_of::<winuser::WINDOWPLACEMENT>() as u32;
         winuser::GetWindowPlacement(self.window.0, &mut placement);
-        self.maximized.replace(placement.showCmd == (winuser::SW_SHOWMAXIMIZED as u32));
+        window_state.maximized = placement.showCmd == (winuser::SW_SHOWMAXIMIZED as u32);
         let saved_window_info = window_state.saved_window_info.as_ref().unwrap();
 
         (saved_window_info.style, saved_window_info.ex_style)
     }
 
-    unsafe fn restore_saved_window(&self) {
+    unsafe fn restore_saved_window(&self, window_state_lock: &mut WindowState) {
         let (rect, mut style, ex_style) = {
-            let mut window_state_lock = self.window_state.lock().unwrap();
-
             // 'saved_window_info' can be None if the window has never been
             // in fullscreen mode before this method gets called.
             if window_state_lock.saved_window_info.is_none() {
@@ -537,8 +515,8 @@ impl Window {
         let window = self.window.clone();
         let window_state = Arc::clone(&self.window_state);
 
-        let maximized = self.maximized.get();
+        let resizable = window_state_lock.resizable;
-        let resizable = self.resizable.get();
+        let maximized = window_state_lock.maximized;
 
         // We're restoring the window to its size and position from before being fullscreened.
         // `ShowWindow` resizes the window, so it must be called from the main thread.
@@ -585,6 +563,7 @@ impl Window {
 
     #[inline]
     pub fn set_fullscreen(&self, monitor: Option<RootMonitorId>) {
+        let mut window_state_lock = self.window_state.lock().unwrap();
         unsafe {
             match &monitor {
                 &Some(RootMonitorId { ref inner }) => {
@@ -593,8 +572,7 @@ impl Window {
                     let window = self.window.clone();
                     let window_state = Arc::clone(&self.window_state);
 
-                    let (style, ex_style) = self.set_fullscreen_style();
+                    let (style, ex_style) = self.set_fullscreen_style(&mut window_state_lock);
-
                     self.events_loop_proxy.execute_in_thread(move |_| {
                         let _ = Self::grab_cursor_inner(&window, false);
 
@@ -634,27 +612,23 @@ impl Window {
                     });
                 }
                 &None => {
-                    self.restore_saved_window();
+                    self.restore_saved_window(&mut window_state_lock);
                 }
             }
         }
 
-        self.fullscreen.replace(monitor);
+        window_state_lock.fullscreen = monitor;
     }
 
     #[inline]
     pub fn set_decorations(&self, decorations: bool) {
-        if self.decorations.get() == decorations {
+        let mut window_state = self.window_state.lock().unwrap();
-            return;
+        if mem::replace(&mut window_state.decorations, decorations) != decorations {
-        }
-
         let style_flags = (winuser::WS_CAPTION | winuser::WS_THICKFRAME) as LONG;
         let ex_style_flags = (winuser::WS_EX_WINDOWEDGE) as LONG;
 
-        // if we are in fullscreen mode, we only change the saved window info
+            // if we are in fullscreen mode, we only change the saved window info
-        if self.fullscreen.borrow().is_some() {
+            if window_state.fullscreen.is_some() {
-            {
-                let mut window_state = self.window_state.lock().unwrap();
                 let saved = window_state.saved_window_info.as_mut().unwrap();
 
                 unsafe {
@@ -677,81 +651,73 @@ impl Window {
                         saved.ex_style as _,
                     );
                 }
-            }
-
-            self.decorations.replace(decorations);
-            return;
-        }
-
-        unsafe {
-            let mut rect: RECT = mem::zeroed();
-            winuser::GetWindowRect(self.window.0, &mut rect);
-
-            let mut style = winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE);
-            let mut ex_style = winuser::GetWindowLongW(self.window.0, winuser::GWL_EXSTYLE);
-            unjust_window_rect(&mut rect, style as _, ex_style as _);
-
-            if decorations {
-                style = style | style_flags;
-                ex_style = ex_style | ex_style_flags;
             } else {
-                style = style & !style_flags;
+                unsafe {
-                ex_style = ex_style & !ex_style_flags;
+                    let mut rect: RECT = mem::zeroed();
+                    winuser::GetWindowRect(self.window.0, &mut rect);
+
+                    let mut style = winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE);
+                    let mut ex_style = winuser::GetWindowLongW(self.window.0, winuser::GWL_EXSTYLE);
+                    unjust_window_rect(&mut rect, style as _, ex_style as _);
+
+                    if decorations {
+                        style = style | style_flags;
+                        ex_style = ex_style | ex_style_flags;
+                    } else {
+                        style = style & !style_flags;
+                        ex_style = ex_style & !ex_style_flags;
+                    }
+
+                    let window = self.window.clone();
+
+                    self.events_loop_proxy.execute_in_thread(move |_| {
+                        winuser::SetWindowLongW(window.0, winuser::GWL_STYLE, style);
+                        winuser::SetWindowLongW(window.0, winuser::GWL_EXSTYLE, ex_style);
+                        winuser::AdjustWindowRectEx(&mut rect, style as _, 0, ex_style as _);
+
+                        winuser::SetWindowPos(
+                            window.0,
+                            ptr::null_mut(),
+                            rect.left,
+                            rect.top,
+                            rect.right - rect.left,
+                            rect.bottom - rect.top,
+                            winuser::SWP_ASYNCWINDOWPOS
+                            | winuser::SWP_NOZORDER
+                            | winuser::SWP_NOACTIVATE
+                            | winuser::SWP_FRAMECHANGED,
+                        );
+                    });
+                }
             }
-
-            let window = self.window.clone();
-
-            self.events_loop_proxy.execute_in_thread(move |_| {
-                winuser::SetWindowLongW(window.0, winuser::GWL_STYLE, style);
-                winuser::SetWindowLongW(window.0, winuser::GWL_EXSTYLE, ex_style);
-                winuser::AdjustWindowRectEx(&mut rect, style as _, 0, ex_style as _);
-
-                winuser::SetWindowPos(
-                    window.0,
-                    ptr::null_mut(),
-                    rect.left,
-                    rect.top,
-                    rect.right - rect.left,
-                    rect.bottom - rect.top,
-                    winuser::SWP_ASYNCWINDOWPOS
-                    | winuser::SWP_NOZORDER
-                    | winuser::SWP_NOACTIVATE
-                    | winuser::SWP_FRAMECHANGED,
-                );
-            });
         }
-
-        self.decorations.replace(decorations);
     }
 
     #[inline]
     pub fn set_always_on_top(&self, always_on_top: bool) {
-        if self.always_on_top.get() == always_on_top {
+        let mut window_state = self.window_state.lock().unwrap();
-            return;
+        if mem::replace(&mut window_state.always_on_top, always_on_top) != always_on_top {
+            let window = self.window.clone();
+            self.events_loop_proxy.execute_in_thread(move |_| {
+                let insert_after = if always_on_top {
+                    winuser::HWND_TOPMOST
+                } else {
+                    winuser::HWND_NOTOPMOST
+                };
+                unsafe {
+                    winuser::SetWindowPos(
+                        window.0,
+                        insert_after,
+                        0,
+                        0,
+                        0,
+                        0,
+                        winuser::SWP_ASYNCWINDOWPOS | winuser::SWP_NOMOVE | winuser::SWP_NOSIZE,
+                    );
+                    winuser::UpdateWindow(window.0);
+                }
+            });
         }
-
-        let window = self.window.clone();
-        self.events_loop_proxy.execute_in_thread(move |_| {
-            let insert_after = if always_on_top {
-                winuser::HWND_TOPMOST
-            } else {
-                winuser::HWND_NOTOPMOST
-            };
-            unsafe {
-                winuser::SetWindowPos(
-                    window.0,
-                    insert_after,
-                    0,
-                    0,
-                    0,
-                    0,
-                    winuser::SWP_ASYNCWINDOWPOS | winuser::SWP_NOMOVE | winuser::SWP_NOSIZE,
-                );
-                winuser::UpdateWindow(window.0);
-            }
-        });
-
-        self.always_on_top.replace(always_on_top);
     }
 
     #[inline]
@@ -771,7 +737,7 @@ impl Window {
         } else {
             icon::unset_for_window(self.window.0, IconType::Small);
         }
-        self.window_icon.replace(window_icon);
+        self.window_state.lock().unwrap().window_icon = window_icon;
     }
 
     #[inline]
@@ -784,7 +750,7 @@ impl Window {
         } else {
             icon::unset_for_window(self.window.0, IconType::Big);
         }
-        self.taskbar_icon.replace(taskbar_icon);
+        self.window_state.lock().unwrap().taskbar_icon = taskbar_icon;
     }
 
     #[inline]
@@ -809,15 +775,25 @@ impl Drop for Window {
 #[derive(Clone)]
 pub struct WindowWrapper(HWND, HDC);
 
-// Send is not implemented for HWND and HDC, we have to wrap it and implement it manually.
+// Send and Sync are not implemented for HWND and HDC, we have to wrap it and implement them manually.
 // For more info see:
 // https://github.com/retep998/winapi-rs/issues/360
 // https://github.com/retep998/winapi-rs/issues/396
+unsafe impl Sync for WindowWrapper {}
 unsafe impl Send for WindowWrapper {}
 
-pub unsafe fn adjust_size(physical_size: PhysicalSize, style: DWORD, ex_style: DWORD) -> (LONG, LONG) {
+pub unsafe fn adjust_size(
+    physical_size: PhysicalSize,
+    style: DWORD,
+    ex_style: DWORD,
+) -> (LONG, LONG) {
     let (width, height): (u32, u32) = physical_size.into();
-    let mut rect = RECT { left: 0, right: width as LONG, top: 0, bottom: height as LONG };
+    let mut rect = RECT {
+        left: 0,
+        right: width as LONG,
+        top: 0,
+        bottom: height as LONG,
+    };
     winuser::AdjustWindowRectEx(&mut rect, style, 0, ex_style);
     (rect.right - rect.left, rect.bottom - rect.top)
 }
@@ -1029,6 +1005,13 @@ unsafe fn init(
             mouse_in_window: false,
             saved_window_info: None,
             dpi_factor,
+            fullscreen: attributes.fullscreen.clone(),
+            window_icon,
+            taskbar_icon,
+            decorations: attributes.decorations,
+            maximized: attributes.maximized,
+            resizable: attributes.resizable,
+            always_on_top: attributes.always_on_top,
         };
         // Creating a mutex to track the current window state
         Arc::new(Mutex::new(window_state))
@@ -1048,14 +1031,7 @@ unsafe fn init(
 
     let win = Window {
         window: real_window,
-        window_state: window_state,
+        window_state,
-        decorations: Cell::new(attributes.decorations),
-        maximized: Cell::new(attributes.maximized.clone()),
-        resizable: Cell::new(attributes.resizable.clone()),
-        fullscreen: RefCell::new(attributes.fullscreen.clone()),
-        always_on_top: Cell::new(attributes.always_on_top),
-        window_icon: Cell::new(window_icon),
-        taskbar_icon: Cell::new(taskbar_icon),
         events_loop_proxy,
     };
 
@@ -1112,7 +1088,6 @@ unsafe fn register_window_class(
     class_name
 }
 
-
 struct ComInitialized(*mut ());
 impl Drop for ComInitialized {
     fn drop(&mut self) {