Improve safety around `HasRawWindowHandle`

[maroider]

We got a bit sidetracked by this in #70, so I'm opening this issue to track it separately.

To summarize the problem briefly:
Letting users use windows with crates like wgpu without requiring the user to write any unsafe code is desirable.
TrustedWindowHandle can't really be used safely after it has been around for a bit. Unfortunately, there's no guarantee that the underlying window handle won't become invalid while a TrustedWindowHandle is alive.
gfx-rs/wgpu#1463 has a more in-depth outline of the issue at hand, and suggests that raw-window-handle should add the following impls of HasRawWindowHandle:

impl<'a, T: HasRawWindowHandle> HasRawWindowHandle for &'a T { /* .. */ }
impl<T: HasRawWindowHandle> HasRawWindowHandle for Rc<T> { /* .. */ }
impl<T: HasRawWindowHandle> HasRawWindowHandle for Arc<T> { /* .. */ }

The addition of these impls of HasRawWindowHandle makes TrustedWindowHandle seem kind of useless (for lack of a less biting word).

[Lokathor]

At a glance, I don't understand what the extra HasRawWindowHandle impls accomplish, because don't any of those values just deref to the contained type when necessary anyway?

[maroider]

It's not explicitly documented as a guarantee of HasRawWindowHandle, but winit and sdl2's window structs destroy the underlying window when they're dropped.
wgpu wants to make sure that a wgpu::Surface is always valid, and as such they need to make sure the Window isn't destroyed while the Surface is alive, because there's no guarantee in the spec that a Vulkan surface will keep the underlying window handle valid.
Surface could just expose a method that allows you to access the underlying window, but that's also not the most convenient thing in the world.
The extra impls allow for convenient access to the underlying window type without having to go through such accessors. Glium is a particularly bad example here (even though Glium doesn't use raw-window-handle) in that you need to go through two layers of accessors to get at the winit::window::Window.

[maroider]

And I forgot to do this earlier, but cc @pythonesque @kvark

[Lokathor]

Hm, well those are all reasonable impls to have, so I'm for it if that fixes things for wgpu.

[pythonesque]

Yep, that proposal still stands--having these implemented would be really helpful!

[pythonesque]

It's not explicitly documented as a guarantee of HasRawWindowHandle, but winit and sdl2's window structs destroy the underlying window when they're dropped.

It's not documented, but from the existing requirements on the HasRawWindowHandle trait, we can infer something related: as long as the window isn't dropped, any implementor of HasRawWindowHandle has to keep any handle it handed out alive. So that's one of the main reasons for the proposal to work like that, it mean no new requirements for downstream implementors of the trait.

[Lokathor]

One related thing: I heard that on Android the window's handle doesn't change but you're only allowed to use it at specific times. is that a concern here?

[pythonesque]

One related thing: I heard that on Android the window's handle doesn't change but you're only allowed to use it at specific times. is that a concern here?

If it is, then I think it needs to be reflected in other APIs (for stuff that actually use the window). But IIRC this is not an issue in practice on Android (can't remember the details).

[pythonesque]

I realized this while responding to a comment from someone else, but I'm not sure the Arc<T> implementation is actually safe with the current unsafe trait implementation, thanks to Arc::make_mut and friends. I think for it to be safe, we'd need something like pinning, it might have to be done for like Arc<UnpinWrapper<T>>, with the HasRawWindowHandle API taking Pin<&Self> rather than self. Which is a little more annoying to use but I think in practice usable in exactly the same way in all the cases where this is safe. This also applies to Rc<T>. (Note that you can create an Arc or Rc as pinned which is the correct way to use this API).

Here UnpinWrapper would just prevent people from making the window Unpin.

[Lokathor]

I mean if you're concerned about a window somehow altering its handle value through &mut self (which is already a violation of the unsafe contract), then don't forget that it can alter its handle value through &self as well.

[pythonesque]

I mean if you're concerned about a window somehow altering its handle value through &mut self (which is already a violation of the unsafe contract), then don't forget that it can alter its handle value through &self as well.

No, I am worried about people swapping out the window for something else and then dropping the window, which is easy to do when you have &mut self and can't be prevented by any contract. Which is pretty much the whole reason Pin exists. OTOH invalidating the handle through mutations to &self violate the contract.

[pythonesque]

Actually wait, hang on a second, I am dumb. Obviously if you were to do this you can't rely on the unsafe contract anymore, since it only applies as long as the original window is alive, so the pinning stuff probably isn't necessary. Or at the very least, it's more of a language lawyering issue than anything else (what does "is alive" mean).

That being said, the pin version of the API is definitely more obviously safe, since it guarantees that the window can't move after the value is called... I actually think with some slight changes to the HasRawWindowHandle contract, we could even avoid needing to specifically opt in Rc and Arc like this. But that has to be weighed against it being annoying to change the existing APIs.

[msiglreith]

One related thing: I heard that on Android the window's handle doesn't change but you're only allowed to use it at specific times. is that a concern here?

To clarify, between suspend/resume the ANativeWindow which is returned as window handle may change (device dependent).