From 0976a9a6a48179218f221ee5c5271e4eb72f1660 Mon Sep 17 00:00:00 2001 From: Micah Johnston Date: Sat, 16 Dec 2023 19:37:21 -0600 Subject: [PATCH] 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 | 14 +++++---- src/macos/view.rs | 36 ++++++++++------------ src/macos/window.rs | 72 +++++++++++++++++++++---------------------- 3 files changed, 60 insertions(+), 62 deletions(-) diff --git a/src/macos/keyboard.rs b/src/macos/keyboard.rs index 7e41c95..18f3bfe 100644 --- a/src/macos/keyboard.rs +++ b/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, } /// 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 { + pub(crate) fn process_native_event(&self, event: id) -> Option { 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(); - self.last_mods = raw_mods; + let any_down = raw_mods.bits() & !self.last_mods.get().bits(); + self.last_mods.set(raw_mods); if is_modifier_code(code) { if any_down == 0 { KeyState::Up diff --git a/src/macos/view.rs b/src/macos/view.rs index 4eaa374..0354e04 100644 --- a/src/macos/view.rs +++ b/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 { - WindowState::from_field(this) - }; + let state = unsafe { WindowState::from_view(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 { - WindowState::from_field(this) - }; + let state = unsafe { WindowState::from_view(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 { - WindowState::from_field(this) - }; + let state = unsafe { WindowState::from_view(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() { - state.window_info = new_window_info; + if new_window_info.physical_size() != window_info.physical_size() { + 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); } diff --git a/src/macos/window.rs b/src/macos/window.rs index e6185b9..e9f4685 100644 --- a/src/macos/window.rs +++ b/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_handler, + window: RefCell::new(window), + window_handler: RefCell::new(window_handler), keyboard_state: KeyboardState::new(), - frame_timer: None, + frame_timer: Cell::new(None), retain_count_after_build, - window_info, - _parent_handle: parent_handle, + window_info: Cell::new(window_info), + _parent_handle: Cell::new(parent_handle), })); unsafe { - (*(*window_state_ptr).window.ns_view) - .set_ivar(BASEVIEW_STATE_IVAR, window_state_ptr as *mut c_void); + (*ns_view).set_ivar(BASEVIEW_STATE_IVAR, window_state_ptr as *const c_void); WindowState::setup_timer(window_state_ptr); } @@ -317,34 +319,32 @@ impl Window { } pub(super) struct WindowState { - window: Window, - window_handler: Box, + window: RefCell, + window_handler: RefCell>, keyboard_state: KeyboardState, - frame_timer: Option, - _parent_handle: Option, + frame_timer: Cell>, + _parent_handle: Cell>, pub retain_count_after_build: usize, /// The last known window info for this window. - pub window_info: WindowInfo, + pub window_info: Cell, } impl WindowState { - /// Returns a mutable reference to a WindowState from an Objective-C field - /// - /// Don't use this to create two simulataneous references to a single - /// 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); + /// Returns a reference to the `WindowState` held by a given `NSView` + pub(super) unsafe fn from_view(view: &Object) -> &Self { + let state_ptr: *const c_void = *view.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 { - self.window_handler.on_event(&mut crate::Window::new(&mut self.window), event) + pub(super) fn trigger_event(&self, event: Event) -> EventStatus { + 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) { - self.window_handler.on_frame(&mut crate::Window::new(&mut self.window)); + pub(super) fn trigger_frame(&self) { + 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 { + pub(super) fn process_native_key_event(&self, event: *mut Object) -> Option { 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.frame_timer = Some(timer); + (*window_state_ptr).frame_timer.set(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 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); } }