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.

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

API soundness issues #238

Closed
parasyte opened this issue Dec 6, 2021 · 3 comments · Fixed by #399
Closed

API soundness issues #238

parasyte opened this issue Dec 6, 2021 · 3 comments · Fixed by #399
Labels
bug Something isn't working

Comments

@parasyte
Copy link
Owner

parasyte commented Dec 6, 2021

This was mentioned to me yesterday by @Lokathor. Trying to recreate a Pixels at runtime (e.g. in response to an event) causes validation errors and crashes.

I found an ugly workaround using Option<Pixels> and calling take() on it to drop the internal references before trying to recreate it. To me, this suggests the bug is related to gfx-rs/wgpu#1463 (and this very long thread: rust-windowing/raw-window-handle#71). The conversation is a bit hard to follow, but I think I can summarize it as: RawWindowHandle needs to be treated like a raw pointer that references the HasRawWindowHandle implementer. In this case, Window is the implementor, and Surface holds a RawWindowHandle pointing to it. This means that their lifetimes are required to be bound together, but there is nothing in the API contract enforcing that. It's left up to the caller of unsafe surface creation:

let surface = unsafe { instance.create_surface(self.surface_texture.window) };

Whoops.

Pixels holds ownership of a surface, and it looks like having two surfaces alive pointing to the same RawWindowHandle is bad news bears. (This is just a best guess.)

Similarly, our API allows dropping the window after creating Pixels, and that is very clearly unsound. We can't borrow the window, since that would prevent moving it later (even though it's perfectly fine to do so). Almost every user moves their window into the event-loop closure to call window.request_redraw() on it, at a minimum.

I think the best way to solve this on our end is to take ownership of the window. That would prevent the issue as reported (can't recreate Pixels without creating a new Window) and it also solves this other related issue (can't drop Window without dropping Pixels). I don't know if shared ownership (with Rc or Arc) is acceptable, since that would probably just result in validation errors and crashes again if multiple surfaces are created from one window.

@parasyte parasyte added the bug Something isn't working label Dec 6, 2021
@zetanumbers
Copy link

So we should mark SurfaceTexture::new as unsafe for now, right?

@Lokathor
Copy link

That's a breaking change that's quick and dirty. If you're going to issue a breaking change you probably are better served to fix the whole problem in one go.

@parasyte
Copy link
Owner Author

I do agree with the last comment. It seems like it should not be much more work to take ownership of the window. The advantage is that it would keep the API safe.

I would love to see a solution for gfx-rs/wgpu#1463 that we could take immediate advantage of but seems like it won't be available soon. @zetanumbers if you need a fix for this bug, I can expedite the work on the window ownership proposal. I'm also open to other suggestions.

parasyte added a commit that referenced this issue Jul 17, 2023
- Adds a lifetime parameter to `Pixels` which is linked to the `HRWH` that is used for creating a `SurfaceTexture`.
- The lifetime parameter is basically incompatible with `winit`s inversion of control, but can be used with `async_winit` (or another wrapper crate that papers over the inversion of control issue).
- Fixes #238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants