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

MonitorHandle and VideoMode can become invalid leading to panics #3258

Open
1 of 3 tasks
daxpedda opened this issue Dec 6, 2023 · 1 comment
Open
1 of 3 tasks

MonitorHandle and VideoMode can become invalid leading to panics #3258

daxpedda opened this issue Dec 6, 2023 · 1 comment
Labels
S - api Design and usability

Comments

@daxpedda
Copy link
Member

daxpedda commented Dec 6, 2023

This is an issue in MonitorHandle and VideoMode. In #3303 (comment) @madsmtm has pointed out that on MacOS these handles are not connected to EventLoop in any way, so they should be fine. We should try and cover all backends and make sure handle method can't and can live after the event loop is exited or otherwise come up with an alternative, e.g. adding a lifetime.

  • Rename VideoMode to VideoModeHandle, similar to other types, to represent that they don't just hold static data: Rename VideoMode to VideoModeHandle #3328.
  • Both MonitorHandle and VideoMode can become invalid, e.g. a monitor gets disconnected. So all methods should return an error to handle that.
  • AFAIK on Linux these types are connected to the EventLoop so we have to come up with a good solution. This is a similar problem to WindowHandle is not sound #3317.

Cc #971 and #2646.


Original Windows only issue

The MonitorHandle on Windows holds a HMONITOR, which can become invalid if e.g. the monitor is disconnected. Or to be more precise, the Windows documentation recommends to listen to WM_DISPLAYCHANGE, which can potentially invalidate any HMONITOR.

All of the MonitorHandle methods call the GetMonitorInfo function and just unwrap(), which would cause a panic if a HMONITOR has become invalid.

See #3255, which only fixed MonitorHandle::video_modes().

@kchibisov
Copy link
Member

This is true for literally any backend. The same with other resources. It's kind of known problem and we generally. have a bunch of PRs around to not panic in monitor handle.

@kchibisov kchibisov added S - api Design and usability and removed DS - windows labels Dec 6, 2023
@daxpedda daxpedda changed the title Windows: MonitorHandle can become invalid leading to panics MonitorHandle and VideoMode can become invalid leading to panics Dec 26, 2023
@daxpedda daxpedda added this to the Version 0.30.0 milestone Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - api Design and usability
Development

No branches or pull requests

2 participants