On Windows, avoid panic in video_modes()

[fornwall]

We are seeing a panic due to the unwrap() usage in video_modes() in crash reporting.

Haven't been able to reproduce it ourselves, but in general GetMonitorInfoW can fail, so this changes MonitorHandle::video_modes() to return a Result.

[kchibisov]

Just return an empty iterator, it's already a thing with some backends.

[fornwall]

Just return an empty iterator, it's already a thing with some backends.

👍 I'll go ahead and update the PR with that.

[daxpedda]

Just return an empty iterator, it's already a thing with some backends.

Isn't this just a problem with Windows returning an error and Winit exposing that to the user? I agree that if the error is just not finding any windows we should indeed return an empty iterator. But if there is an error we can return to the user shouldn't we do that?

Though it seems to exclusively affect Windows.

@fornwall did your crash reports show what kind of error Windows is returning here? Because the documentation doesn't mention what kind of error this function can return.

[fornwall]

did your crash reports show what kind of error Windows is returning here?

This is the available information:

panic: called Result::unwrap() on an Err value: Os { code: 1461, kind: Uncategorized, message: "Invalid monitor handle." }

Windows 11(22621), Intel(R) UHD Graphics 630, driver version 0.404.2111.

[daxpedda]

I know absolutely nothing about the Windows backend, but doesn't that point to a bug in Winit then?

[daxpedda]

Taking a look at the HMONITOR documentation, my first guess would be that the monitor handle might have been invalidated by the time this function is called.

So, again, I know nothing about the Windows backend or the API, my guess would be that if we can check the validity of the handle we should do that first, if not, we can check for that error. In either case an empty iterator would be better then returning a Result.

[fornwall]

Should we move the get_monitor_info() call out of the loop?

[fornwall]

Just return an empty iterator, it's already a thing with some backends.

Updated to log an error and break out of the loop now.

[kchibisov]

Can't we write something like that

diff --git a/src/platform_impl/windows/monitor.rs b/src/platform_impl/windows/monitor.rs
index f1313970..1548bc05 100644
--- a/src/platform_impl/windows/monitor.rs
+++ b/src/platform_impl/windows/monitor.rs
@@ -230,44 +230,45 @@ impl MonitorHandle {
         // EnumDisplaySettingsExW can return duplicate values (or some of the
         // fields are probably changing, but we aren't looking at those fields
         // anyway), so we're using a BTreeSet deduplicate
-        let mut modes = BTreeSet::new();
-        let mut i = 0;
+        let mut modes = BTreeSet::<RootVideoMode>::new();
+        let mod_map = |mode: RootVideoMode| mode.video_mode;
+
+        let monitor_info = match get_monitor_info(self.0) {
+            Ok(monitor_info) => monitor_info,
+            Err(error) => {
+                log::warn!("Error from get_monitor_info: {error}");
+                return modes.into_iter().map(mod_map);
+            }
+        };
+
+        let device_name = monitor_info.szDevice.as_ptr();
 
+        let mut i = 0;
         loop {
-            unsafe {
-                match get_monitor_info(self.0) {
-                    Ok(monitor_info) => {
-                        let device_name = monitor_info.szDevice.as_ptr();
-                        let mut mode: DEVMODEW = mem::zeroed();
-                        mode.dmSize = mem::size_of_val(&mode) as u16;
-                        if EnumDisplaySettingsExW(device_name, i, &mut mode, 0) == false.into() {
-                            break;
-                        }
-                        i += 1;
-
-                        const REQUIRED_FIELDS: u32 =
-                            DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT | DM_DISPLAYFREQUENCY;
-                        assert!(has_flag(mode.dmFields, REQUIRED_FIELDS));
-
-                        // Use Ord impl of RootVideoMode
-                        modes.insert(RootVideoMode {
-                            video_mode: VideoMode {
-                                size: (mode.dmPelsWidth, mode.dmPelsHeight),
-                                bit_depth: mode.dmBitsPerPel as u16,
-                                refresh_rate_millihertz: mode.dmDisplayFrequency * 1000,
-                                monitor: self.clone(),
-                                native_video_mode: Box::new(mode),
-                            },
-                        });
-                    }
-                    Err(error) => {
-                        log::warn!("Error from get_monitor_info: {error}");
-                        break;
-                    }
-                }
+            let mut mode: DEVMODEW = unsafe { mem::zeroed() };
+            mode.dmSize = mem::size_of_val(&mode) as u16;
+            if unsafe { EnumDisplaySettingsExW(device_name, i, &mut mode, 0) } == false.into() {
+                break;
             }
+
+            const REQUIRED_FIELDS: u32 =
+                DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT | DM_DISPLAYFREQUENCY;
+            assert!(has_flag(mode.dmFields, REQUIRED_FIELDS));
+
+            // Use Ord impl of RootVideoMode
+            modes.insert(RootVideoMode {
+                video_mode: VideoMode {
+                    size: (mode.dmPelsWidth, mode.dmPelsHeight),
+                    bit_depth: mode.dmBitsPerPel as u16,
+                    refresh_rate_millihertz: mode.dmDisplayFrequency * 1000,
+                    monitor: self.clone(),
+                    native_video_mode: Box::new(mode),
+                },
+            });
+
+            i += 1;
         }
 
-        modes.into_iter().map(|mode| mode.video_mode)
+        modes.into_iter().map(mod_map)
     }
 }

[fornwall]

Can't we write something like that

@kchibisov Looks like a good improvement -have update the PR with that!