macOS: use interior mutability for WindowState (#157)

Forming mutable references to the WindowState is unsound given the possibility
of reentrant calls to NSView methods. Instead, form only immutable references
to the WindowState and wrap mutable fields in Cell and RefCell.

Follow-up work should use try_borrow_mut instead of borrow_mut to avoid
panicking in the case of reentrant calls.

src/macos/keyboard.rs

@@ -18,6 +18,8 @@
 
 //! Conversion of platform keyboard event into cross-platform event.
 
+use std::cell::Cell;
+
 use cocoa::appkit::{NSEvent, NSEventModifierFlags, NSEventType};
 use cocoa::base::id;
 use cocoa::foundation::NSString;
@@ -44,7 +46,7 @@ pub(crate) fn from_nsstring(s: id) -> String {
 /// Most of the logic in this module is adapted from Mozilla, and in particular
 /// TextInputHandler.mm.
 pub(crate) struct KeyboardState {
-    last_mods: NSEventModifierFlags,
+    last_mods: Cell<NSEventModifierFlags>,
 }
 
 /// Convert a macOS platform key code (keyCode field of NSEvent).
@@ -269,15 +271,15 @@ fn is_modifier_code(code: Code) -> bool {
 
 impl KeyboardState {
     pub(crate) fn new() -> KeyboardState {
-        let last_mods = NSEventModifierFlags::empty();
+        let last_mods = Cell::new(NSEventModifierFlags::empty());
         KeyboardState { last_mods }
     }
 
     pub(crate) fn last_mods(&self) -> NSEventModifierFlags {
-        self.last_mods
+        self.last_mods.get()
     }
 
-    pub(crate) fn process_native_event(&mut self, event: id) -> Option<KeyboardEvent> {
+    pub(crate) fn process_native_event(&self, event: id) -> Option<KeyboardEvent> {
         unsafe {
             let event_type = event.eventType();
             let key_code = event.keyCode();
@@ -292,8 +294,8 @@ impl KeyboardState {
                     // We use `bits` here because we want to distinguish the
                     // device dependent bits (when both left and right keys
                     // may be pressed, for example).
-                    let any_down = raw_mods.bits() & !self.last_mods.bits();
+                    let any_down = raw_mods.bits() & !self.last_mods.get().bits();
-                    self.last_mods = raw_mods;
+                    self.last_mods.set(raw_mods);
                     if is_modifier_code(code) {
                         if any_down == 0 {
                             KeyState::Up

src/macos/view.rs

@@ -33,9 +33,7 @@ macro_rules! add_simple_mouse_class_method {
     ($class:ident, $sel:ident, $event:expr) => {
         #[allow(non_snake_case)]
         extern "C" fn $sel(this: &Object, _: Sel, _: id){
-            let state: &mut WindowState = unsafe {
+            let state = unsafe { WindowState::from_view(this) };
-                WindowState::from_field(this)
-            };
 
             state.trigger_event(Event::Mouse($event));
         }
@@ -53,9 +51,7 @@ macro_rules! add_mouse_button_class_method {
     ($class:ident, $sel:ident, $event_ty:ident, $button:expr) => {
         #[allow(non_snake_case)]
         extern "C" fn $sel(this: &Object, _: Sel, event: id){
-            let state: &mut WindowState = unsafe {
+            let state = unsafe { WindowState::from_view(this) };
-                WindowState::from_field(this)
-            };
 
             let modifiers = unsafe { NSEvent::modifierFlags(event) };
 
@@ -76,9 +72,7 @@ macro_rules! add_simple_keyboard_class_method {
     ($class:ident, $sel:ident) => {
         #[allow(non_snake_case)]
         extern "C" fn $sel(this: &Object, _: Sel, event: id){
-            let state: &mut WindowState = unsafe {
+            let state = unsafe { WindowState::from_view(this) };
-                WindowState::from_field(this)
-            };
 
             if let Some(key_event) = state.process_native_key_event(event){
                 let status = state.trigger_event(Event::Keyboard(key_event));
@@ -230,7 +224,7 @@ extern "C" fn release(this: &mut Object, _sel: Sel) {
         let state_ptr: *mut c_void = *this.get_ivar(BASEVIEW_STATE_IVAR);
 
         if !state_ptr.is_null() {
-            let retain_count_after_build = WindowState::from_field(this).retain_count_after_build;
+            let retain_count_after_build = WindowState::from_view(this).retain_count_after_build;
 
             if retain_count <= retain_count_after_build {
                 WindowState::stop_and_free(this);
@@ -263,7 +257,7 @@ extern "C" fn view_did_change_backing_properties(this: &Object, _: Sel, _: id) {
         let scale_factor: f64 =
             if ns_window.is_null() { 1.0 } else { NSWindow::backingScaleFactor(ns_window) };
 
-        let state: &mut WindowState = WindowState::from_field(this);
+        let state = WindowState::from_view(this);
 
         let bounds: NSRect = msg_send![this, bounds];
 
@@ -272,10 +266,12 @@ extern "C" fn view_did_change_backing_properties(this: &Object, _: Sel, _: id) {
             scale_factor,
         );
 
+        let window_info = state.window_info.get();
+
         // Only send the event when the window's size has actually changed to be in line with the
         // other platform implementations
-        if new_window_info.physical_size() != state.window_info.physical_size() {
+        if new_window_info.physical_size() != window_info.physical_size() {
-            state.window_info = new_window_info;
+            state.window_info.set(new_window_info);
             state.trigger_event(Event::Window(WindowEvent::Resized(new_window_info)));
         }
     }
@@ -361,7 +357,7 @@ extern "C" fn update_tracking_areas(this: &Object, _self: Sel, _: id) {
 }
 
 extern "C" fn mouse_moved(this: &Object, _sel: Sel, event: id) {
-    let state: &mut WindowState = unsafe { WindowState::from_field(this) };
+    let state = unsafe { WindowState::from_view(this) };
 
     let point: NSPoint = unsafe {
         let point = NSEvent::locationInWindow(event);
@@ -379,7 +375,7 @@ extern "C" fn mouse_moved(this: &Object, _sel: Sel, event: id) {
 }
 
 extern "C" fn scroll_wheel(this: &Object, _: Sel, event: id) {
-    let state: &mut WindowState = unsafe { WindowState::from_field(this) };
+    let state = unsafe { WindowState::from_view(this) };
 
     let delta = unsafe {
         let x = NSEvent::scrollingDeltaX(event) as f32;
@@ -428,7 +424,7 @@ fn get_drop_data(sender: id) -> DropData {
     }
 }
 
-fn on_event(window_state: &mut WindowState, event: MouseEvent) -> NSUInteger {
+fn on_event(window_state: &WindowState, event: MouseEvent) -> NSUInteger {
     let event_status = window_state.trigger_event(Event::Mouse(event));
     match event_status {
         EventStatus::AcceptDrop(DropEffect::Copy) => NSDragOperationCopy,
@@ -440,7 +436,7 @@ fn on_event(window_state: &mut WindowState, event: MouseEvent) -> NSUInteger {
 }
 
 extern "C" fn dragging_entered(this: &Object, _sel: Sel, sender: id) -> NSUInteger {
-    let state: &mut WindowState = unsafe { WindowState::from_field(this) };
+    let state = unsafe { WindowState::from_view(this) };
     let modifiers = state.keyboard_state().last_mods();
     let drop_data = get_drop_data(sender);
 
@@ -454,7 +450,7 @@ extern "C" fn dragging_entered(this: &Object, _sel: Sel, sender: id) -> NSUInteg
 }
 
 extern "C" fn dragging_updated(this: &Object, _sel: Sel, sender: id) -> NSUInteger {
-    let state: &mut WindowState = unsafe { WindowState::from_field(this) };
+    let state = unsafe { WindowState::from_view(this) };
     let modifiers = state.keyboard_state().last_mods();
     let drop_data = get_drop_data(sender);
 
@@ -475,7 +471,7 @@ extern "C" fn prepare_for_drag_operation(_this: &Object, _sel: Sel, _sender: id)
 }
 
 extern "C" fn perform_drag_operation(this: &Object, _sel: Sel, sender: id) -> BOOL {
-    let state: &mut WindowState = unsafe { WindowState::from_field(this) };
+    let state = unsafe { WindowState::from_view(this) };
     let modifiers = state.keyboard_state().last_mods();
     let drop_data = get_drop_data(sender);
 
@@ -493,7 +489,7 @@ extern "C" fn perform_drag_operation(this: &Object, _sel: Sel, sender: id) -> BO
 }
 
 extern "C" fn dragging_exited(this: &Object, _sel: Sel, _sender: id) {
-    let state: &mut WindowState = unsafe { WindowState::from_field(this) };
+    let state = unsafe { WindowState::from_view(this) };
 
     on_event(state, MouseEvent::DragLeft);
 }

src/macos/window.rs

@@ -1,3 +1,4 @@
+use std::cell::{Cell, RefCell};
 use std::ffi::c_void;
 use std::marker::PhantomData;
 use std::ptr;
@@ -254,19 +255,20 @@ impl Window {
 
         let retain_count_after_build: usize = unsafe { msg_send![window.ns_view, retainCount] };
 
+        let ns_view = window.ns_view;
+
         let window_state_ptr = Box::into_raw(Box::new(WindowState {
-            window,
+            window: RefCell::new(window),
-            window_handler,
+            window_handler: RefCell::new(window_handler),
             keyboard_state: KeyboardState::new(),
-            frame_timer: None,
+            frame_timer: Cell::new(None),
             retain_count_after_build,
-            window_info,
+            window_info: Cell::new(window_info),
-            _parent_handle: parent_handle,
+            _parent_handle: Cell::new(parent_handle),
         }));
 
         unsafe {
-            (*(*window_state_ptr).window.ns_view)
+            (*ns_view).set_ivar(BASEVIEW_STATE_IVAR, window_state_ptr as *const c_void);
-                .set_ivar(BASEVIEW_STATE_IVAR, window_state_ptr as *mut c_void);
 
             WindowState::setup_timer(window_state_ptr);
         }
@@ -317,34 +319,32 @@ impl Window {
 }
 
 pub(super) struct WindowState {
-    window: Window,
+    window: RefCell<Window>,
-    window_handler: Box<dyn WindowHandler>,
+    window_handler: RefCell<Box<dyn WindowHandler>>,
     keyboard_state: KeyboardState,
-    frame_timer: Option<CFRunLoopTimer>,
+    frame_timer: Cell<Option<CFRunLoopTimer>>,
-    _parent_handle: Option<ParentHandle>,
+    _parent_handle: Cell<Option<ParentHandle>>,
     pub retain_count_after_build: usize,
     /// The last known window info for this window.
-    pub window_info: WindowInfo,
+    pub window_info: Cell<WindowInfo>,
 }
 
 impl WindowState {
-    /// Returns a mutable reference to a WindowState from an Objective-C field
+    /// Returns a reference to the `WindowState` held by a given `NSView`
-    ///
+    pub(super) unsafe fn from_view(view: &Object) -> &Self {
-    /// Don't use this to create two simulataneous references to a single
+        let state_ptr: *const c_void = *view.get_ivar(BASEVIEW_STATE_IVAR);
-    /// WindowState. Apparently, macOS blocks for the duration of an event,
-    /// callback, meaning that this shouldn't be a problem in practice.
-    pub(super) unsafe fn from_field(obj: &Object) -> &mut Self {
-        let state_ptr: *mut c_void = *obj.get_ivar(BASEVIEW_STATE_IVAR);
 
-        &mut *(state_ptr as *mut Self)
+        &*(state_ptr as *const Self)
     }
 
-    pub(super) fn trigger_event(&mut self, event: Event) -> EventStatus {
+    pub(super) fn trigger_event(&self, event: Event) -> EventStatus {
-        self.window_handler.on_event(&mut crate::Window::new(&mut self.window), event)
+        let mut window = self.window.borrow_mut();
+        self.window_handler.borrow_mut().on_event(&mut crate::Window::new(&mut window), event)
     }
 
-    pub(super) fn trigger_frame(&mut self) {
+    pub(super) fn trigger_frame(&self) {
-        self.window_handler.on_frame(&mut crate::Window::new(&mut self.window));
+        let mut window = self.window.borrow_mut();
+        self.window_handler.borrow_mut().on_frame(&mut crate::Window::new(&mut window));
 
         let mut do_close = false;
 
@@ -360,14 +360,15 @@ impl WindowState {
         */
 
         // Check if the user requested the window to close
-        if self.window.close_requested {
+        if window.close_requested {
             do_close = true;
-            self.window.close_requested = false;
+            window.close_requested = false;
         }
 
         if do_close {
             unsafe {
-                if let Some(ns_window) = self.window.ns_window.take() {
+                let ns_window = self.window.borrow_mut().ns_window.take();
+                if let Some(ns_window) = ns_window {
                     ns_window.close();
                 } else {
                     // FIXME: How do we close a non-parented window? Is this even
@@ -381,15 +382,15 @@ impl WindowState {
         &self.keyboard_state
     }
 
-    pub(super) fn process_native_key_event(&mut self, event: *mut Object) -> Option<KeyboardEvent> {
+    pub(super) fn process_native_key_event(&self, event: *mut Object) -> Option<KeyboardEvent> {
         self.keyboard_state.process_native_event(event)
     }
 
     /// Don't call until WindowState pointer is stored in view
-    unsafe fn setup_timer(window_state_ptr: *mut WindowState) {
+    unsafe fn setup_timer(window_state_ptr: *const WindowState) {
         extern "C" fn timer_callback(_: *mut __CFRunLoopTimer, window_state_ptr: *mut c_void) {
             unsafe {
-                let window_state = &mut *(window_state_ptr as *mut WindowState);
+                let window_state = &*(window_state_ptr as *const WindowState);
 
                 window_state.trigger_frame();
             }
@@ -407,18 +408,16 @@ impl WindowState {
 
         CFRunLoop::get_current().add_timer(&timer, kCFRunLoopDefaultMode);
 
-        let window_state = &mut *(window_state_ptr);
+        (*window_state_ptr).frame_timer.set(Some(timer));
-
-        window_state.frame_timer = Some(timer);
     }
 
     /// Call when freeing view
     pub(super) unsafe fn stop_and_free(ns_view_obj: &mut Object) {
-        let state_ptr: *mut c_void = *ns_view_obj.get_ivar(BASEVIEW_STATE_IVAR);
+        let state_ptr: *const c_void = *ns_view_obj.get_ivar(BASEVIEW_STATE_IVAR);
 
         // Take back ownership of Box<WindowState> so that it gets dropped
         // when it goes out of scope
-        let mut window_state = Box::from_raw(state_ptr as *mut WindowState);
+        let window_state = Box::from_raw(state_ptr as *mut WindowState);
 
         if let Some(frame_timer) = window_state.frame_timer.take() {
             CFRunLoop::get_current().remove_timer(&frame_timer, kCFRunLoopDefaultMode);
@@ -432,7 +431,8 @@ impl WindowState {
         window_state.trigger_event(Event::Window(WindowEvent::WillClose));
 
         // If in non-parented mode, we want to also quit the app altogether
-        if let Some(app) = window_state.window.ns_app.take() {
+        let app = window_state.window.borrow_mut().ns_app.take();
+        if let Some(app) = app {
             app.stop_(app);
         }
     }