Skip to content

Commit

Permalink
Remove unsound SendSyncWrapper
Browse files Browse the repository at this point in the history
  • Loading branch information
daxpedda committed Dec 24, 2023
1 parent 4f6fd44 commit e79d4ce
Show file tree
Hide file tree
Showing 20 changed files with 313 additions and 248 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ Unreleased` header.
- **Breaking:** On Web, return `RawWindowHandle::WebCanvas` instead of `RawWindowHandle::Web`.
- **Breaking:** On Web, macOS and iOS, return `HandleError::Unavailable` when a window handle is not available.
- **Breaking:** Bump MSRV from `1.65` to `1.70`.
- **Breaking:** Remove `WindowAttributes::fullscreen()` and expose as field directly.

# 0.29.5

Expand Down
15 changes: 0 additions & 15 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,18 +182,3 @@ mod platform_impl;
pub mod window;

pub mod platform;

/// Wrapper for objects which winit will access on the main thread so they are effectively `Send`
/// and `Sync`, since they always execute on a single thread.
///
/// # Safety
///
/// Winit can run only one event loop at a time, and the event loop itself is tied to some thread.
/// The objects could be sent across the threads, but once passed to winit, they execute on the
/// main thread if the platform demands it. Thus, marking such objects as `Send + Sync` is safe.
#[doc(hidden)]
#[derive(Clone, Debug)]
pub(crate) struct SendSyncWrapper<T>(pub(crate) T);

unsafe impl<T> Send for SendSyncWrapper<T> {}
unsafe impl<T> Sync for SendSyncWrapper<T> {}
3 changes: 1 addition & 2 deletions src/platform/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use crate::event_loop::EventLoopWindowTarget;
use crate::platform_impl::PlatformCustomCursorBuilder;
use crate::window::CustomCursor;
use crate::window::{Window, WindowBuilder};
use crate::SendSyncWrapper;

use web_sys::HtmlCanvasElement;

Expand Down Expand Up @@ -85,7 +84,7 @@ pub trait WindowBuilderExtWebSys {

impl WindowBuilderExtWebSys for WindowBuilder {
fn with_canvas(mut self, canvas: Option<HtmlCanvasElement>) -> Self {
self.platform_specific.canvas = SendSyncWrapper(canvas);
self.platform_specific.set_canvas(canvas);
self
}

Expand Down
2 changes: 1 addition & 1 deletion src/platform_impl/ios/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ impl WinitUIWindow {

this.setRootViewController(Some(view_controller));

match window_attributes.fullscreen.0.clone().map(Into::into) {
match window_attributes.fullscreen.clone().map(Into::into) {
Some(Fullscreen::Exclusive(ref video_mode)) => {
let monitor = video_mode.monitor();
let screen = monitor.ui_screen(mtm);
Expand Down
21 changes: 11 additions & 10 deletions src/platform_impl/ios/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,15 +369,6 @@ impl Inner {
rwh_06::RawWindowHandle::UiKit(window_handle)
}

#[cfg(feature = "rwh_06")]
pub fn raw_display_handle_rwh_06(
&self,
) -> Result<rwh_06::RawDisplayHandle, rwh_06::HandleError> {
Ok(rwh_06::RawDisplayHandle::UiKit(
rwh_06::UiKitDisplayHandle::new(),
))
}

pub fn theme(&self) -> Option<Theme> {
warn!("`Window::theme` is ignored on iOS");
None
Expand Down Expand Up @@ -426,7 +417,7 @@ impl Window {
// TODO: transparency, visible

let main_screen = UIScreen::main(mtm);
let fullscreen = window_attributes.fullscreen.0.clone().map(Into::into);
let fullscreen = window_attributes.fullscreen.clone().map(Into::into);
let screen = match fullscreen {
Some(Fullscreen::Exclusive(ref video_mode)) => video_mode.monitor.ui_screen(mtm),
Some(Fullscreen::Borderless(Some(ref monitor))) => monitor.ui_screen(mtm),
Expand Down Expand Up @@ -534,6 +525,16 @@ impl Window {
Err(rwh_06::HandleError::Unavailable)
}
}

#[cfg(feature = "rwh_06")]
#[inline]
pub(crate) fn raw_display_handle_rwh_06(
&self,
) -> Result<rwh_06::RawDisplayHandle, rwh_06::HandleError> {
Ok(rwh_06::RawDisplayHandle::UiKit(
rwh_06::UiKitDisplayHandle::new(),
))
}
}

// WindowExtIOS
Expand Down
2 changes: 1 addition & 1 deletion src/platform_impl/linux/wayland/window/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ impl Window {
window_state.set_resizable(attributes.resizable);

// Set startup mode.
match attributes.fullscreen.0.map(Into::into) {
match attributes.fullscreen.map(Into::into) {
Some(Fullscreen::Exclusive(_)) => {
warn!("`Fullscreen::Exclusive` is ignored on Wayland");
}
Expand Down
6 changes: 3 additions & 3 deletions src/platform_impl/linux/x11/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ impl UnownedWindow {
let xconn = &event_loop.xconn;
let atoms = xconn.atoms();
#[cfg(feature = "rwh_06")]
let root = match window_attrs.parent_window.0 {
let root = match window_attrs.parent_window.as_ref().map(|handle| handle.0) {
Some(rwh_06::RawWindowHandle::Xlib(handle)) => handle.window as xproto::Window,
Some(rwh_06::RawWindowHandle::Xcb(handle)) => handle.window.get(),
Some(raw) => unreachable!("Invalid raw window handle {raw:?} on X11"),
Expand Down Expand Up @@ -556,10 +556,10 @@ impl UnownedWindow {
if window_attrs.maximized {
leap!(window.set_maximized_inner(window_attrs.maximized)).ignore_error();
}
if window_attrs.fullscreen.0.is_some() {
if window_attrs.fullscreen.is_some() {
if let Some(flusher) =
leap!(window
.set_fullscreen_inner(window_attrs.fullscreen.0.clone().map(Into::into)))
.set_fullscreen_inner(window_attrs.fullscreen.clone().map(Into::into)))
{
flusher.ignore_error()
}
Expand Down
10 changes: 7 additions & 3 deletions src/platform_impl/macos/monitor.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
#![allow(clippy::unnecessary_cast)]

use std::{collections::VecDeque, fmt};
use std::{
collections::VecDeque,
fmt,
sync::{Arc, Mutex},
};

use core_foundation::{
array::{CFArrayGetCount, CFArrayGetValueAtIndex},
Expand All @@ -23,7 +27,7 @@ pub struct VideoMode {
bit_depth: u16,
refresh_rate_millihertz: u32,
pub(crate) monitor: MonitorHandle,
pub(crate) native_mode: NativeDisplayMode,
pub(crate) native_mode: Arc<Mutex<NativeDisplayMode>>,
}

impl PartialEq for VideoMode {
Expand Down Expand Up @@ -290,7 +294,7 @@ impl MonitorHandle {
refresh_rate_millihertz,
bit_depth,
monitor: monitor.clone(),
native_mode: NativeDisplayMode(mode),
native_mode: Arc::new(Mutex::new(NativeDisplayMode(mode))),
}
})
}
Expand Down
37 changes: 26 additions & 11 deletions src/platform_impl/macos/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use std::collections::VecDeque;
use std::f64;
use std::ops;
use std::sync::PoisonError;
use std::sync::{Mutex, MutexGuard};

use crate::{
Expand Down Expand Up @@ -105,6 +106,20 @@ impl Window {
Err(rwh_06::HandleError::Unavailable)
}
}

#[cfg(feature = "rwh_06")]
#[inline]
pub(crate) fn raw_display_handle_rwh_06(
&self,
) -> Result<rwh_06::RawDisplayHandle, rwh_06::HandleError> {
struct UnsafeSendWrapper<T>(T);

unsafe impl<T> Send for UnsafeSendWrapper<T> {}

Ok(self
.maybe_wait_on_main(|w| UnsafeSendWrapper(w.raw_display_handle_rwh_06()))
.0)
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
Expand Down Expand Up @@ -280,7 +295,7 @@ impl WinitWindow {
trace_scope!("WinitWindow::new");

let this = autoreleasepool(|_| {
let screen = match attrs.fullscreen.0.clone().map(Into::into) {
let screen = match attrs.fullscreen.clone().map(Into::into) {
Some(Fullscreen::Borderless(Some(monitor)))
| Some(Fullscreen::Exclusive(VideoMode { monitor, .. })) => {
monitor.ns_screen(mtm).or_else(|| NSScreen::mainScreen(mtm))
Expand Down Expand Up @@ -440,7 +455,7 @@ impl WinitWindow {
.ok_or_else(|| os_error!(OsError::CreationError("Couldn't create `NSWindow`")))?;

#[cfg(feature = "rwh_06")]
match attrs.parent_window.0 {
match attrs.parent_window.map(|handle| handle.0) {
Some(rwh_06::RawWindowHandle::AppKit(handle)) => {
// SAFETY: Caller ensures the pointer is valid or NULL
// Unwrap is fine, since the pointer comes from `NonNull`.
Expand Down Expand Up @@ -516,14 +531,14 @@ impl WinitWindow {
}
}

let delegate = WinitWindowDelegate::new(&this, attrs.fullscreen.0.is_some());
let delegate = WinitWindowDelegate::new(&this, attrs.fullscreen.is_some());

// XXX Send `Focused(false)` right after creating the window delegate, so we won't
// obscure the real focused events on the startup.
delegate.queue_event(WindowEvent::Focused(false));

// Set fullscreen mode after we setup everything
this.set_fullscreen(attrs.fullscreen.0.map(Into::into));
this.set_fullscreen(attrs.fullscreen.map(Into::into));

// Setting the window as key has to happen *after* we set the fullscreen
// state, since otherwise we'll briefly see the window at normal size
Expand Down Expand Up @@ -1088,7 +1103,11 @@ impl WinitWindow {
unsafe {
let result = ffi::CGDisplaySetDisplayMode(
display_id,
video_mode.native_mode.0,
video_mode
.native_mode
.lock()
.unwrap_or_else(PoisonError::into_inner)
.0,
std::ptr::null(),
);
assert!(result == ffi::kCGErrorSuccess, "failed to set video mode");
Expand Down Expand Up @@ -1363,12 +1382,8 @@ impl WinitWindow {

#[cfg(feature = "rwh_06")]
#[inline]
pub fn raw_display_handle_rwh_06(
&self,
) -> Result<rwh_06::RawDisplayHandle, rwh_06::HandleError> {
Ok(rwh_06::RawDisplayHandle::AppKit(
rwh_06::AppKitDisplayHandle::new(),
))
pub fn raw_display_handle_rwh_06(&self) -> rwh_06::RawDisplayHandle {
rwh_06::RawDisplayHandle::AppKit(rwh_06::AppKitDisplayHandle::new())
}

fn toggle_style_mask(&self, mask: NSWindowStyleMask, on: bool) {
Expand Down
5 changes: 4 additions & 1 deletion src/platform_impl/web/async/dispatcher.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use super::super::main_thread::MainThreadMarker;

use super::{channel, AsyncReceiver, AsyncSender, Wrapper};
use std::{
cell::Ref,
Expand All @@ -10,10 +12,11 @@ struct Closure<T>(Box<dyn FnOnce(&T) + Send>);

impl<T> Dispatcher<T> {
#[track_caller]
pub fn new(value: T) -> Option<(Self, DispatchRunner<T>)> {
pub fn new(main_thread: MainThreadMarker, value: T) -> Option<(Self, DispatchRunner<T>)> {
let (sender, receiver) = channel::<Closure<T>>();

Wrapper::new(
main_thread,
value,
|value, Closure(closure)| {
// SAFETY: The given `Closure` here isn't really `'static`, so we shouldn't do anything
Expand Down
6 changes: 4 additions & 2 deletions src/platform_impl/web/async/waker.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::super::main_thread::MainThreadMarker;
use super::Wrapper;
use atomic_waker::AtomicWaker;
use std::future;
Expand All @@ -19,7 +20,7 @@ struct Sender(Arc<Inner>);

impl<T> WakerSpawner<T> {
#[track_caller]
pub fn new(value: T, handler: fn(&T, usize)) -> Option<Self> {
pub fn new(main_thread: MainThreadMarker, value: T, handler: fn(&T, usize)) -> Option<Self> {
let inner = Arc::new(Inner {
counter: AtomicUsize::new(0),
waker: AtomicWaker::new(),
Expand All @@ -31,6 +32,7 @@ impl<T> WakerSpawner<T> {
let sender = Sender(Arc::clone(&inner));

let wrapper = Wrapper::new(
main_thread,
handler,
|handler, count| {
let handler = handler.borrow();
Expand Down Expand Up @@ -86,7 +88,7 @@ impl<T> WakerSpawner<T> {

pub fn fetch(&self) -> usize {
debug_assert!(
self.0.is_main_thread(),
MainThreadMarker::new().is_some(),
"this should only be called from the main thread"
);

Expand Down
54 changes: 10 additions & 44 deletions src/platform_impl/web/async/wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ use std::cell::{Ref, RefCell};
use std::future::Future;
use std::marker::PhantomData;
use std::sync::Arc;
use wasm_bindgen::prelude::wasm_bindgen;
use wasm_bindgen::{JsCast, JsValue};

use super::super::main_thread::MainThreadMarker;

// Unsafe wrapper type that allows us to use `T` when it's not `Send` from other threads.
// `value` **must** only be accessed on the main thread.
Expand Down Expand Up @@ -34,36 +34,15 @@ unsafe impl<const SYNC: bool, V> Send for Value<SYNC, V> {}
unsafe impl<V> Sync for Value<true, V> {}

impl<const SYNC: bool, V, S: Clone + Send, E> Wrapper<SYNC, V, S, E> {
thread_local! {
static MAIN_THREAD: bool = {
#[wasm_bindgen]
extern "C" {
#[derive(Clone)]
type Global;

#[wasm_bindgen(method, getter, js_name = Window)]
fn window(this: &Global) -> JsValue;
}

let global: Global = js_sys::global().unchecked_into();
!global.window().is_undefined()
};
}

#[track_caller]
pub fn new<R: Future<Output = ()>>(
_: MainThreadMarker,
value: V,
handler: fn(&RefCell<Option<V>>, E),
receiver: impl 'static + FnOnce(Arc<RefCell<Option<V>>>) -> R,
sender_data: S,
sender_handler: fn(&S, E),
) -> Option<Self> {
Self::MAIN_THREAD.with(|safe| {
if !safe {
panic!("only callable from inside the `Window`")
}
});

let value = Arc::new(RefCell::new(Some(value)));

wasm_bindgen_futures::spawn_local({
Expand All @@ -86,29 +65,16 @@ impl<const SYNC: bool, V, S: Clone + Send, E> Wrapper<SYNC, V, S, E> {
}

pub fn send(&self, event: E) {
Self::MAIN_THREAD.with(|is_main_thread| {
if *is_main_thread {
(self.handler)(&self.value.value, event)
} else {
(self.sender_handler)(&self.sender_data, event)
}
})
}

pub fn is_main_thread(&self) -> bool {
Self::MAIN_THREAD.with(|is_main_thread| *is_main_thread)
if MainThreadMarker::new().is_some() {
(self.handler)(&self.value.value, event)
} else {
(self.sender_handler)(&self.sender_data, event)
}
}

pub fn value(&self) -> Option<Ref<'_, V>> {
Self::MAIN_THREAD.with(|is_main_thread| {
if *is_main_thread {
Some(Ref::map(self.value.value.borrow(), |value| {
value.as_ref().unwrap()
}))
} else {
None
}
})
MainThreadMarker::new()
.map(|_| Ref::map(self.value.value.borrow(), |value| value.as_ref().unwrap()))
}

pub fn with_sender_data<T>(&self, f: impl FnOnce(&S) -> T) -> T {
Expand Down
Loading

0 comments on commit e79d4ce

Please sign in to comment.