Safe surface creation

[daxpedda]

Now that we use raw-window-handle v0.6 (#4202), safe Surface creation should be possible. This is done by adding a lifetime to Surface that is bound to the passed window.

Additionally I added Surface::create_surface_from_raw() which still takes a HasWindowHandle + HasDisplayHandle but remains unsafe and returns a Surface<'static>.

Fixes #1463.

[notgull]

If you parameterize the Surface by the actual window type, you can avoid the weird 'static mutation and instead just pass around the Surface wrapping an Arc<Window> or something. See the softbuffer version of this PR for more info (rust-windowing/softbuffer#132)

[daxpedda]

If you parameterize the Surface by the actual window type, you can avoid the weird 'static mutation and instead just pass around the Surface wrapping an Arc<Window> or something. See the softbuffer version of this PR for more info (rust-windowing/softbuffer#132)

I don't think this would work as well in Wgpu as it did in Softbuffer. Wgpu has a bunch of methods that can create a Surface from pointers, which I would argue isn't desirable: Surface<*mut std::ffi::c_void> (we would also need to solve the Send + Sync problem then). We could of course in cases like this return a Surface<()>, but at that point I don't see how Surface<'static> is worse.

Maybe I'm missing something here, let me know.

[daxpedda]

Rebased on trunk.

[kpreid]

What about the “raw_window_handle must be a valid object to create a surface upon” part of the safety contract? Are we sure that there is no possible state of the window, on all platforms, that will not be surprising to wgpu and thereby cause unsound misbehavior?

[daxpedda]

What about the “raw_window_handle must be a valid object to create a surface upon” part of the safety contract? Are we sure that there is no possible state of the window, on all platforms, that will not be surprising to wgpu and thereby cause unsound misbehavior?

My assumption here is that's what raw-window-handle intended here, but I have really no knowledge about this and couldn't find anything mentioned in the documentation.

@notgull would you be able to comment on that?

[notgull]

I don't think this would work as well in Wgpu as it did in Softbuffer. Wgpu has a bunch of methods that can create a Surface from pointers, which I would argue isn't desirable: Surface<*mut std::ffi::c_void> (we would also need to solve the Send + Sync problem then).

This happens in softbuffer sometimes too, in this case we return a Display<NoDisplayHandle> or a Surface<NoWindowHandle>. Something similar should be able to happen in wgpu.

We could of course in cases like this return a Surface<()>, but at that point I don't see how Surface<'static> is worse.

Surface<'static> is worse because it lies about its lifetime requirements, imparting unsoundness on the rest of the program.

My assumption here is that's what raw-window-handle intended here, but I have really no knowledge about this and couldn't find anything mentioned in the documentation.

This is true; while a WindowHandle is alive the window is guaranteed to be alive right alongside it, like with AsFd.

[daxpedda]

I don't think this would work as well in Wgpu as it did in Softbuffer. Wgpu has a bunch of methods that can create a Surface from pointers, which I would argue isn't desirable: Surface<*mut std::ffi::c_void> (we would also need to solve the Send + Sync problem then).

This happens in softbuffer sometimes too, in this case we return a Display<NoDisplayHandle> or a Surface<NoWindowHandle>. Something similar should be able to happen in wgpu.

I tried this idea, but again, Wgpu doesn't make this straightforward. I have to pass down the generic now somehow in all the functions that take Surface. E.g. Instance::request_adapter() takes a RequestAdapterOptions which holds &Surface. I can't pass this down easily, because DynContext is supposed to be object safe, adding any generics to methods breaks that. Adding a generic to DynContext itself seems a bit too messy for me. Lifetimes don't have that problem because you can just use anonymous ones.

I could continue to play around with all this, but would like to hear a word from a member if this is desired in the first place or not.

[daxpedda]

Rebased on trunk, no changes.

[notgull]

I think I used dynamic dispatch and 'static when I tried this in #3932 to get around this. It reduces functionality but will probably work, at least in the short term.

[daxpedda]

Rebased on master.

[cwfitzgerald]

So making surface creation safe is something we definitely want, I don't think this is quite the way to do it though.

Notably, we only actually need the very surface wgpu layer be safe - wgpu-core can purely address the raw window handle assuming that the layer above it is handling it correctly.

We should have create_surface be:

trait WgpuSurfaceRequirement: HasWindowHandle + HasDisplayHandle {}

pub fn create_surface<'a, W>(&self, window: W) -> Result<Surface<'a>, CreateSurfaceError>
where
     W: WgpuSurfaceRequirement + 'a,

Note that there isn't actually a generic here. We can store W in a Option<Box<dyn WgpuSurfaceRequirement + 'a>> (minding the send/sync requirements of wasm/non-wasm). For the functions that take a raw pointer to create a surface, we put None in that slot and make the lifetime static.

This prevents the generics from infecting user code, and allows the flexibility to store both references and owned values as W, behaving as expected.

wgpu-core and below behave like they did before with raw handles everywhere.

[cwfitzgerald]

Please re-request a review from me once the changes are addressed to make sure I see it!

[daxpedda]

@cwfitzgerald I'm unsure what exactly your request is.

I looked over my changes again, but I don't believe I introduced any generics. Maybe you were just pointing out that we don't want to introduce a generic based on the conversation I had with nogull before?

In any case, the current implementation seems to be exactly what you are requesting, including not touching anything else then wgpu. The only difference I can see is the WgpuSurfaceRequirement trait, which wasn't necessary as we don't actually store the given window, but only the surface we extract from it.

Apologies for any misunderstanding.

[daxpedda]

Rebased on master.
Only CHANGELOG.md changed.

[cwfitzgerald]

Code is good, just style and docs.

[cwfitzgerald]

LGTM after comment!

[cwfitzgerald]

Hit the wrong button!

[daxpedda]

Just rebased onto trunk again.
No changes.

[cwfitzgerald]

Nice!