Skip to content

Commit

Permalink
macOS: use interior mutability for WindowState
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@glowcoil
glowcoil committed Dec 16, 2023
1 parent 348bc9f commit f4cfcd1
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 63 deletions.
14 changes: 8 additions & 6 deletions src/macos/keyboard.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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).
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand Down
36 changes: 16 additions & 20 deletions src/macos/view.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand All @@ -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) };

Expand All @@ -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));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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];

Expand All @@ -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)));
}
}
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);
}
74 changes: 37 additions & 37 deletions src/macos/window.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::cell::{Cell, RefCell};
use std::ffi::c_void;
use std::marker::PhantomData;
use std::ptr;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -317,34 +319,32 @@ impl Window {
}

pub(super) struct WindowState {
window: Window,
window_handler: Box<dyn WindowHandler>,
window: RefCell<Window>,
window_handler: RefCell<Box<dyn WindowHandler>>,
keyboard_state: KeyboardState,
frame_timer: Option<CFRunLoopTimer>,
_parent_handle: Option<ParentHandle>,
frame_timer: Cell<Option<CFRunLoopTimer>>,
_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
///
/// 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);

&mut *(state_ptr as *mut Self)
/// 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);

&*(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;

Expand All @@ -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
Expand All @@ -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();
}
Expand All @@ -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<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);
Expand All @@ -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);
}
}
Expand Down

0 comments on commit f4cfcd1

Please sign in to comment.