Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs: Improve documentation for safe window handles #114

Closed
wants to merge 1 commit into from
Closed

docs: Improve documentation for safe window handles #114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 113 additions & 44 deletions src/borrowed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//! Borrowable window handles based on the ones in this crate.
//!
//! These should be 100% safe to pass around and use, no possibility of dangling or
//! invalidity.
//! These should be 100% safe to pass around and use, no possibility of dangling or invalidity.

use core::fmt;
use core::marker::PhantomData;
Expand All @@ -10,15 +9,63 @@ use crate::{HasRawDisplayHandle, HasRawWindowHandle, RawDisplayHandle, RawWindow

/// The application is currently active.
///
/// This structure is a token that indicates that the application is
/// not presently suspended. It is used to ensure that the window handle
/// is only used when the application is active.
/// This structure is a token that indicates that the application is not presently suspended. It is used
/// to ensure that the window handle is only used when the application is active.
///
/// It is safe to create this token on platforms where the application
/// is guaranteed to be active, such as on desktop platforms. On Android
/// platforms, the application may be suspended, so this token must be
/// either created with `unsafe` or derived from a `HasDisplayHandle`
/// type.
/// It is safe to create this token on platforms where the application is guaranteed to be active, such as
/// on desktop platforms. On Android platforms, the application may be suspended, so this token must be
/// either created with `unsafe` or derived from a `HasDisplayHandle` type.
///
/// # Motivating Use Case
///
/// On Android, there is an [Activity]-global [`ANativeWindow`] object that is used for drawing. This
/// handle is used [within the `RawWindowHandle` type] for Android NDK, since it is necessary for GFX
/// APIs to draw to the screen.
///
/// However, the [`ANativeWindow`] type can be arbitrarily invalidated by the underlying Android runtime.
/// The reasoning for this is complicated, but this idea is exposed to native code through the
/// [`onNativeWindowCreated`] and [`onNativeWindowDestroyed`] callbacks. To save you a click, the
/// conditions associated with these callbacks are:
///
/// - [`onNativeWindowCreated`] provides a valid [`ANativeWindow`] pointer that can be used for drawing.
/// - [`onNativeWindowDestroyed`] indicates that the previous [`ANativeWindow`] pointer is no longer
/// valid. The documentation clarifies that, *once the function returns*, the [`ANativeWindow`] pointer
/// can no longer be used for drawing without resulting in undefined behavior.
///
/// In [`winit`], these are exposed via the [`Resumed`] and [`Suspended`] events, respectively. Therefore,
/// between the last [`Suspended`] event and the next [`Resumed`] event, it is undefined behavior to use
/// the raw window handle. This condition makes it tricky to define an API that safely wraps the raw
/// window handles, since an existing window handle can be made invalid at any time.
///
/// The `Active` struct works around this by providing a borrow-checker enforced guarantee that the
/// application is not suspended. This takes advantage of the fact that he current callback-based event
/// loop setup for [`winit`], [`sdl2`] and other event loop systems to my knowledge requires an
/// `&mut self` reference to the event loop. This means that it is not possible to borrow the event loop
/// and poll for events at the same.
Comment on lines +40 to +44
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, realized this may not work with Window implementing HasDisplayHandle. Might need to rethink either implementing HasDisplayHandle on Window, or something different...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't the display that the window shows up within have the actual display?

///
/// Therefore, the `Active` token acts as a guarantee that "for the current event loop iteration, the
/// event loop is not suspended". Since we know that the event loop isn't suspended, we can successfully
/// borrow the display/window handle and use it for drawing.
///
/// Note that, while the window handle on non-Android platforms can be invalidated, they can't be
/// invalidated in a way that causes undefined behavior. Therefore, it is safe to create an `Active` token
/// in all contexts using the [`Active::new`] method no matter what.
///
/// Another possible way of enforcing these guarantees would be to have an `is_valid` method on
/// [`HasWindowHandle`] to test if its window is currently valid. However, this would require the user
/// to check if the window is valid every time they want to use it, which may create an unnecessary
/// performance hit. The `Active` token is a more ergonomic solution to this problem.
///
/// [Activity]: https://developer.android.com/reference/android/app/Activity
/// [`ANativeWindow`]: https://developer.android.com/ndk/reference/group/a-native-window
/// [within the `RawWindowHandle` type]: struct.AndroidNdkWindowHandle.html#structfield.a_native_window
/// [`onNativeWindowCreated`]: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks#onnativewindowcreated
/// [`onNativeWindowDestroyed`]: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks#onnativewindowdestroyed
/// [`winit`]: https://crates.io/crates/winit
/// [`Resumed`]: https://docs.rs/winit/latest/winit/event/enum.Event.html#variant.Resumed
/// [`Suspended`]: https://docs.rs/winit/latest/winit/event/enum.Event.html#variant.Suspended
/// [`sdl2`]: https://crates.io/crates/sdl2
/// [`Active::new`]: struct.Active.html#method.new
pub struct Active<'a> {
_marker: PhantomData<&'a *const ()>,
}
Expand All @@ -34,7 +81,16 @@ impl<'a> Active<'a> {
///
/// # Safety
///
/// The application must not be `Suspend`ed.
/// On Android platforms, the native window handle must be available. It is unsound to call this
/// function between the [`onNativeWindowDestroyed`] and [`onNativeWindowCreated`] callbacks.
/// Otherwise, a dangling reference to the [`ANativeWindow`] will be created.
///
/// On other platforms, this function is safe to call. The safe [`Active::new`] function should be
/// used instead.
///
/// [`onNativeWindowCreated`]: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks#onnativewindowcreated
/// [`onNativeWindowDestroyed`]: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks#onnativewindowdestroyed
/// [`ANativeWindow`]: https://developer.android.com/ndk/reference/group/a-native-window
///
/// # Examples
///
Expand All @@ -50,11 +106,10 @@ impl<'a> Active<'a> {
}
}

/// Create a new active token on a system where the application is
/// guaranteed to be active.
/// Create a new active token on a system where the application is guaranteed to be active.
///
/// On most platforms, there is no event where the application is
/// suspended, so there are no cases where this function is unsafe.
/// On most platforms, there is no event where the application is suspended, so there are no cases
/// where this function is unsafe.
///
/// ```
/// use raw_window_handle::Active;
Expand Down Expand Up @@ -92,17 +147,15 @@ impl<'a> Active<'a> {
///
/// # Safety
///
/// The safety requirements of [`HasRawDisplayHandle`] apply here as
/// well. To clarify, the [`DisplayHandle`] must contain a valid window
/// handle for its lifetime. In addition, the handle must be consistent
/// between multiple calls barring platform-specific events.
/// The safety requirements of [`HasRawDisplayHandle`] apply here as well. To reiterate, the
/// [`DisplayHandle`] must contain a valid window handle for its lifetime.
///
/// In addition, the `active` function must only return an `Active`
/// token if the application is active.
/// In addition, the `active` function must only return an [`Active`] token if the application is active.
/// This also implies that the [`Active`] token must be invalidated beween events. See the documentation
/// on the [`Active`] type for more information about these safety requirements.
///
/// Note that these requirements are not enforced on `HasDisplayHandle`,
/// rather, they are enforced on the constructors of [`Active`] and
/// [`DisplayHandle`]. This is because the `HasDisplayHandle` trait is
/// Note that these requirements are not enforced on `HasDisplayHandle`, rather, they are enforced on the
/// constructors of [`Active`] and [`DisplayHandle`]. This is because the `HasDisplayHandle` trait is
/// safe to implement.
///
/// [`HasRawDisplayHandle`]: crate::HasRawDisplayHandle
Expand All @@ -121,7 +174,10 @@ pub trait HasDisplayHandle {

/// The handle to the display controller of the windowing system.
///
/// Get the underlying raw display handle with the `HasRawDisplayHandle` trait.
/// This is the primary return type of the [`HasDisplayHandle`] trait. It is guaranteed to contain
/// a valid platform-specific display handle for its lifetime.
///
/// Get the underlying raw display handle with the [`HasRawDisplayHandle`] trait.
#[repr(transparent)]
#[derive(PartialEq, Eq, Hash)]
pub struct DisplayHandle<'a> {
Expand All @@ -143,13 +199,12 @@ impl<'a> Clone for DisplayHandle<'a> {
}

impl<'a> DisplayHandle<'a> {
/// Borrow a `DisplayHandle` from a `RawDisplayHandle`.
/// Borrow a `DisplayHandle` from a [`RawDisplayHandle`].
///
/// # Safety
///
/// The `RawDisplayHandle` must be valid for the lifetime and the
/// application must be `Active`. See the requirements on the
/// [`HasDisplayHandle`] trait for more information.
/// The `RawDisplayHandle` must be valid for the lifetime and the application must be [`Active`]. See
/// the requirements on the [`HasDisplayHandle`] trait and [`Active`] type for more information.
pub unsafe fn borrow_raw(raw: RawDisplayHandle, active: &Active<'a>) -> Self {
let _ = active;
Self {
Expand All @@ -167,8 +222,7 @@ unsafe impl HasRawDisplayHandle for DisplayHandle<'_> {

impl<'a> HasDisplayHandle for DisplayHandle<'a> {
fn active(&self) -> Option<Active<'_>> {
// SAFETY: The fact that this handle was created means that the
// application is active.
// SAFETY: The fact that this handle was created means that the application is active.
Some(unsafe { Active::new_unchecked() })
}

Expand All @@ -187,15 +241,26 @@ impl<'a> HasDisplayHandle for DisplayHandle<'a> {
///
/// # Safety
///
/// The safety requirements of [`HasRawWindowHandle`] apply here as
/// well. To clarify, the [`WindowHandle`] must contain a valid window
/// handle for its lifetime. In addition, the handle must be consistent
/// between multiple calls barring platform-specific events.
/// All pointers within the resulting [`WindowHandle`] must be valid and not dangling for the lifetime of
/// the handle.
///
/// Note that these requirements are not enforced on `HasWindowHandle`,
/// rather, they are enforced on the constructors of
/// [`WindowHandle`]. This is because the `HasWindowHandle` trait is
/// safe to implement.
/// Note that this guarantee only applies to *pointers*, and not any window ID types in the handle.
/// This includes Window IDs (XIDs) from X11, `HWND`s from Win32, and the window ID for web platforms.
/// There is no way for Rust to enforce any kind of invariant on these types, since:
///
/// - For all three listed platforms, it is possible for safe code in the same process to delete
/// the window.
/// - For X11 and Win32, it is possible for code in a different process to delete the window.
/// - For X11, it is possible for code on a different *machine* to delete the window.
///
/// It is *also* possible for the window to be replaced with another, valid-but-different window. User
/// code should be aware of this possibility, and should be ready to soundly handle the possible error
/// conditions that can arise from this.
///
/// In addition, the window handle must not be invalidated for the duration of the [`Active`] token.
///
/// Note that these requirements are not enforced on `HasWindowHandle`, rather, they are enforced on the
/// constructors of [`WindowHandle`]. This is because the `HasWindowHandle` trait is safe to implement.
pub trait HasWindowHandle {
/// Get a handle to the window.
fn window_handle<'this, 'active>(
Expand All @@ -208,8 +273,13 @@ pub trait HasWindowHandle {

/// The handle to a window.
///
/// This handle is guaranteed to be safe and valid. Get the underlying
/// raw window handle with the `HasRawWindowHandle` trait.
/// This is the primary return type of the [`HasWindowHandle`] trait. All *pointers* within this type
/// are guaranteed to be valid and not dangling for the lifetime of the handle. This excludes window IDs
/// like XIDs, `HWND`s, and the window ID for web platforms. See the documentation on the
/// [`HasWindowHandle`] trait for more information about these safety requirements.
///
/// This handle is guaranteed to be safe and valid. Get the underlying raw window handle with the
/// [`HasRawWindowHandle`] trait.
#[repr(transparent)]
#[derive(PartialEq, Eq, Hash)]
pub struct WindowHandle<'a> {
Expand All @@ -231,12 +301,11 @@ impl<'a> Clone for WindowHandle<'a> {
}

impl<'a> WindowHandle<'a> {
/// Borrow a `WindowHandle` from a `RawWindowHandle`.
/// Borrow a `WindowHandle` from a [`RawWindowHandle`].
///
/// # Safety
///
/// The `RawWindowHandle` must be valid for the lifetime and the
/// application must be `Active`.
/// The [`RawWindowHandle`] must be valid for the lifetime and the application must be `Active`.
pub unsafe fn borrow_raw(raw: RawWindowHandle, active: &Active<'a>) -> Self {
let _ = active;
Self {
Expand Down