Refactor SharedState so that it is no longer behind an Arc (#2573)

* Refactor SharedState so that it is no longer behind an Arc

* Always use `Window::lock_shared_state`

src/platform_impl/macos/util/async.rs

@@ -1,6 +1,5 @@
 use std::mem;
 use std::ops::Deref;
-use std::sync::{Mutex, Weak};
 
 use dispatch::Queue;
 use objc2::foundation::{is_main_thread, CGFloat, NSPoint, NSSize, NSString};
@@ -11,7 +10,7 @@ use crate::{
     platform_impl::platform::{
         appkit::{NSScreen, NSWindow, NSWindowLevel, NSWindowStyleMask},
         ffi,
-        window::{SharedState, SharedStateMutexGuard},
+        window::WinitWindow,
     },
 };
 
@@ -95,13 +94,8 @@ pub(crate) fn set_ignore_mouse_events(window: &NSWindow, ignore: bool) {
 
 // `toggleFullScreen` is thread-safe, but our additional logic to account for
 // window styles isn't.
-pub(crate) fn toggle_full_screen_async(
+pub(crate) fn toggle_full_screen_async(window: Id<WinitWindow, Shared>, not_fullscreen: bool) {
-    window: &NSWindow,
+    let window = MainThreadSafe(window);
-    not_fullscreen: bool,
-    shared_state: Weak<Mutex<SharedState>>,
-) {
-    let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) };
-    let shared_state = MainThreadSafe(shared_state);
     Queue::main().exec_async(move || {
         // `toggleFullScreen` doesn't work if the `StyleMask` is none, so we
         // set a normal style temporarily. The previous state will be
@@ -112,13 +106,9 @@ pub(crate) fn toggle_full_screen_async(
                 NSWindowStyleMask::NSTitledWindowMask | NSWindowStyleMask::NSResizableWindowMask;
             if !curr_mask.contains(required) {
                 set_style_mask(&window, required);
-                if let Some(shared_state) = shared_state.upgrade() {
+                window
-                    let mut shared_state_lock = SharedStateMutexGuard::new(
+                    .lock_shared_state("toggle_full_screen_async")
-                        shared_state.lock().unwrap(),
+                    .saved_style = Some(curr_mask);
-                        "toggle_full_screen_callback",
-                    );
-                    shared_state_lock.saved_style = Some(curr_mask);
-                }
             }
         }
         // Window level must be restored from `CGShieldingWindowLevel()
@@ -141,46 +131,40 @@ pub(crate) unsafe fn restore_display_mode_async(ns_screen: u32) {
 
 // `setMaximized` is not thread-safe
 pub(crate) fn set_maximized_async(
-    window: &NSWindow,
+    window: Id<WinitWindow, Shared>,
     is_zoomed: bool,
     maximized: bool,
-    shared_state: Weak<Mutex<SharedState>>,
 ) {
-    let window = unsafe { MainThreadSafe(mem::transmute::<&NSWindow, &'static NSWindow>(window)) };
+    let window = MainThreadSafe(window);
-    let shared_state = MainThreadSafe(shared_state);
     Queue::main().exec_async(move || {
-        if let Some(shared_state) = shared_state.upgrade() {
+        let mut shared_state = window.lock_shared_state("set_maximized_async");
-            let mut shared_state_lock =
+        // Save the standard frame sized if it is not zoomed
-                SharedStateMutexGuard::new(shared_state.lock().unwrap(), "set_maximized");
+        if !is_zoomed {
+            shared_state.standard_frame = Some(window.frame());
+        }
 
-            // Save the standard frame sized if it is not zoomed
+        shared_state.maximized = maximized;
-            if !is_zoomed {
-                shared_state_lock.standard_frame = Some(window.frame());
-            }
 
-            shared_state_lock.maximized = maximized;
+        if shared_state.fullscreen.is_some() {
+            // Handle it in window_did_exit_fullscreen
+            return;
+        }
 
-            if shared_state_lock.fullscreen.is_some() {
+        if window
-                // Handle it in window_did_exit_fullscreen
+            .styleMask()
-                return;
+            .contains(NSWindowStyleMask::NSResizableWindowMask)
-            }
+        {
-
+            // Just use the native zoom if resizable
-            if window
+            window.zoom(None);
-                .styleMask()
+        } else {
-                .contains(NSWindowStyleMask::NSResizableWindowMask)
+            // if it's not resizable, we set the frame directly
-            {
+            let new_rect = if maximized {
-                // Just use the native zoom if resizable
+                let screen = NSScreen::main().expect("no screen found");
-                window.zoom(None);
+                screen.visibleFrame()
             } else {
-                // if it's not resizable, we set the frame directly
+                shared_state.saved_standard_frame()
-                let new_rect = if maximized {
+            };
-                    let screen = NSScreen::main().expect("no screen found");
+            window.setFrame_display(new_rect, false);
-                    screen.visibleFrame()
-                } else {
-                    shared_state_lock.saved_standard_frame()
-                };
-                window.setFrame_display(new_rect, false);
-            }
         }
     });
 }

src/platform_impl/macos/view.rs

@@ -148,7 +148,7 @@ declare_class!(
         #[sel(initWithId:acceptsFirstMouse:)]
         fn init_with_id(
             &mut self,
-            window: *mut WinitWindow,
+            window: &WinitWindow,
             accepts_first_mouse: bool,
         ) -> Option<&mut Self> {
             let this: Option<&mut Self> = unsafe { msg_send![super(self), init] };
@@ -164,10 +164,7 @@ declare_class!(
                     forward_key_to_app: false,
                 };
 
-                Ivar::write(
+                Ivar::write(&mut this._ns_window, window.retain());
-                    &mut this._ns_window,
-                    unsafe { Id::retain(window) }.expect("non-null window"),
-                );
                 Ivar::write(&mut this.state, Box::new(state));
                 Ivar::write(&mut this.marked_text, NSMutableAttributedString::new());
                 Ivar::write(&mut this.accepts_first_mouse, accepts_first_mouse);

src/platform_impl/macos/window.rs

@@ -4,7 +4,7 @@ use std::{
     os::raw::c_void,
     sync::{
         atomic::{AtomicBool, Ordering},
-        Arc, Mutex, MutexGuard,
+        Mutex, MutexGuard,
     },
 };
 
@@ -105,7 +105,7 @@ declare_class!(
     pub(crate) struct WinitWindow {
         // TODO: Fix unnecessary boxing here
         // SAFETY: These are initialized in WinitWindow::new, right after it is created.
-        shared_state: IvarDrop<Box<Arc<Mutex<SharedState>>>>,
+        shared_state: IvarDrop<Box<Mutex<SharedState>>>,
         decorations: IvarDrop<Box<AtomicBool>>,
     }
 
@@ -169,7 +169,7 @@ pub(crate) struct SharedStateMutexGuard<'a> {
 
 impl<'a> SharedStateMutexGuard<'a> {
     #[inline]
-    pub(crate) fn new(guard: MutexGuard<'a, SharedState>, called_from_fn: &'static str) -> Self {
+    fn new(guard: MutexGuard<'a, SharedState>, called_from_fn: &'static str) -> Self {
         trace!("Locked shared state in `{}`", called_from_fn);
         Self {
             guard,
@@ -301,10 +301,7 @@ impl WinitWindow {
                     maximized: attrs.maximized,
                     ..Default::default()
                 };
-                Ivar::write(
+                Ivar::write(&mut this.shared_state, Box::new(Mutex::new(state)));
-                    &mut this.shared_state,
-                    Box::new(Arc::new(Mutex::new(state))),
-                );
                 Ivar::write(
                     &mut this.decorations,
                     Box::new(AtomicBool::new(attrs.decorations)),
@@ -414,11 +411,11 @@ impl WinitWindow {
         match attrs.preferred_theme {
             Some(theme) => {
                 set_ns_theme(Some(theme));
-                let mut state = this.shared_state.lock().unwrap();
+                let mut state = this.lock_shared_state("WinitWindow::new");
                 state.current_theme = Some(theme);
             }
             None => {
-                let mut state = this.shared_state.lock().unwrap();
+                let mut state = this.lock_shared_state("WinitWindow::new");
                 state.current_theme = Some(get_ns_theme());
             }
         }
@@ -443,6 +440,12 @@ impl WinitWindow {
         Ok((this, delegate))
     }
 
+    pub(super) fn retain(&self) -> Id<WinitWindow, Shared> {
+        // SAFETY: The pointer is valid, and the window is always `Shared`
+        // TODO(madsmtm): Remove the need for unsafety here
+        unsafe { Id::retain(self as *const Self as *mut Self).unwrap() }
+    }
+
     pub(super) fn view(&self) -> Id<WinitView, Shared> {
         // SAFETY: The view inside WinitWindow is always `WinitView`
         unsafe { Id::cast(self.contentView()) }
@@ -826,12 +829,7 @@ impl WinitWindow {
         if is_zoomed == maximized {
             return;
         };
-        util::set_maximized_async(
+        util::set_maximized_async(self.retain(), is_zoomed, maximized);
-            self,
-            is_zoomed,
-            maximized,
-            Arc::downgrade(&*self.shared_state),
-        );
     }
 
     #[inline]
@@ -966,30 +964,18 @@ impl WinitWindow {
 
         match (&old_fullscreen, &fullscreen) {
             (&None, &Some(_)) => {
-                util::toggle_full_screen_async(
+                util::toggle_full_screen_async(self.retain(), old_fullscreen.is_none());
-                    self,
-                    old_fullscreen.is_none(),
-                    Arc::downgrade(&*self.shared_state),
-                );
             }
             (&Some(Fullscreen::Borderless(_)), &None) => {
                 // State is restored by `window_did_exit_fullscreen`
-                util::toggle_full_screen_async(
+                util::toggle_full_screen_async(self.retain(), old_fullscreen.is_none());
-                    self,
-                    old_fullscreen.is_none(),
-                    Arc::downgrade(&*self.shared_state),
-                );
             }
             (&Some(Fullscreen::Exclusive(ref video_mode)), &None) => {
                 unsafe {
                     util::restore_display_mode_async(video_mode.monitor().native_identifier())
                 };
                 // Rest of the state is restored by `window_did_exit_fullscreen`
-                util::toggle_full_screen_async(
+                util::toggle_full_screen_async(self.retain(), old_fullscreen.is_none());
-                    self,
-                    old_fullscreen.is_none(),
-                    Arc::downgrade(&*self.shared_state),
-                );
             }
             (&Some(Fullscreen::Borderless(_)), &Some(Fullscreen::Exclusive(_))) => {
                 // If we're already in fullscreen mode, calling
@@ -1182,8 +1168,7 @@ impl WinitWindow {
 
     #[inline]
     pub fn theme(&self) -> Option<Theme> {
-        let state = self.shared_state.lock().unwrap();
+        self.lock_shared_state("theme").current_theme
-        state.current_theme
     }
 
     #[inline]
@@ -1219,13 +1204,13 @@ impl WindowExtMacOS for WinitWindow {
 
     #[inline]
     fn simple_fullscreen(&self) -> bool {
-        let shared_state_lock = self.shared_state.lock().unwrap();
+        self.lock_shared_state("simple_fullscreen")
-        shared_state_lock.is_simple_fullscreen
+            .is_simple_fullscreen
     }
 
     #[inline]
     fn set_simple_fullscreen(&self, fullscreen: bool) -> bool {
-        let mut shared_state_lock = self.shared_state.lock().unwrap();
+        let mut shared_state_lock = self.lock_shared_state("set_simple_fullscreen");
 
         let app = NSApp();
         let is_native_fullscreen = shared_state_lock.fullscreen.is_some();

src/platform_impl/macos/window_delegate.rs

@@ -57,10 +57,7 @@ declare_class!(
             this.map(|this| {
                 let scale_factor = window.scale_factor();
 
-                Ivar::write(&mut this.window, unsafe {
+                Ivar::write(&mut this.window, window.retain());
-                    let window: *const WinitWindow = window;
-                    Id::retain(window as *mut WinitWindow).unwrap()
-                });
                 Ivar::write(&mut this.initial_fullscreen, initial_fullscreen);
                 Ivar::write(&mut this.previous_position, None);
                 Ivar::write(&mut this.previous_scale_factor, scale_factor);