On Wed, 31 Jan 2024 07:23:13 GMT, Christoph Langer <clan...@openjdk.org> wrote:
>> The assertions reported in the bug were observed spuriously and here and >> there broke tests in some Windows configurations. >> For instance [JDK-8266129](https://bugs.openjdk.org/browse/JDK-8266129), >> [JDK-8269529](https://bugs.openjdk.org/browse/JDK-8269529) or >> [JDK-8323664](https://bugs.openjdk.org/browse/JDK-8323664) came up due to >> this. >> >> The problem is that in Windows environments without a valid display, e.g. >> started by system services or via PowerShell Remoting, one sees a Monitor >> with name 'Windisc' in `EnumDisplayMonitors`. >> However, it seems to be some kind of a pseudo device where you can not get a >> DC via `CreateDC`. This behavior/monitor type doesn't seem to be well >> documented, though. >> >> I hereby modify the device initialization code to only count/detect monitors >> where CreateDC returns non-NULL in Devices.cpp. I also add some more >> checking/error handling to AwtWin32GraphicsDevice::Initialize() for >> correctness. >> >> Furthermore, I re-enable the test >> `javax/swing/reliability/HangDuringStaticInitialization.java` for Windows >> Debug VMs, which reverts the fix from JDK-8269529 that should not be >> necessary any more. > > Christoph Langer has updated the pull request incrementally with one > additional commit since the last revision: > > Add comments src/java.desktop/windows/native/libawt/windows/Devices.cpp line 96: > 94: > 95: // Callback for CountMonitors below > 96: BOOL WINAPI clb_fCountMonitors(HMONITOR hMon, HDC hDC, LPRECT rRect, > LPARAM lP) Can `clb_fCountMonitors` be declared `static`? It's not used from other translation units. src/java.desktop/windows/native/libawt/windows/Devices.cpp line 102: > 100: memset((void*)(&mieInfo), 0, sizeof(MONITORINFOEX)); > 101: mieInfo.cbSize = sizeof(MONITORINFOEX); > 102: if (TRUE == ::GetMonitorInfo(hMon, (LPMONITORINFOEX)(&mieInfo))) { The documentation for [`GetMonitorInfo`](https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmonitorinfow) says, _<q cite="https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmonitorinfow#return-value";>If the function succeeds, the return value is nonzero. If the function fails, the return value is zero.</q>_ Whereas `TRUE == 1`; therefore the condition should be != 0 or rather simply `if (::GetMonitorInfo(/*params*/))`. src/java.desktop/windows/native/libawt/windows/Devices.cpp line 125: > 123: > 124: // Callback for CollectMonitors below > 125: BOOL WINAPI clb_fCollectMonitors(HMONITOR hMon, HDC hDC, LPRECT rRect, > LPARAM lP) Can `clb_fCollectMonitors` be declared `static`? The function is not used from other translation units. src/java.desktop/windows/native/libawt/windows/Devices.cpp line 143: > 141: } else { > 142: J2dTraceLn1(J2D_TRACE_INFO, "Devices::CollectMonitors: > GetMonitorInfo failed for monitor with handle %p", hMon); > 143: } The code to verify the monitor is repeated. To avoid code duplication, I suggest moving it into a helper function `IsValidMonitor` or `CanCreateDCForMonitor` and use the helper in both `clb_fCountMonitors` and `clb_fCollectMonitors`. src/java.desktop/windows/native/libawt/windows/Devices.cpp line 143: > 141: } else { > 142: J2dTraceLn1(J2D_TRACE_INFO, "Devices::CollectMonitors: > GetMonitorInfo failed for monitor with handle %p", hMon); > 143: } I also have a concern for branching… just for logging. In release builds, the body of the `else`-blocks becomes empty and compiler should optimize it out, so it may not affect release builds at all. src/java.desktop/windows/native/libawt/windows/Devices.cpp line 149: > 147: } > 148: > 149: int WINAPI CollectMonitors(HMONITOR* hmpMonitors, int nNum) As far as I can see both `CollectMonitors` and `CountMonitors` above can be declared `static`. src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp line 184: > 182: VERIFY(hBMDC != NULL); > 183: if (hBMDC == NULL) > 184: return; I believe `VERIFY` is redundant now because you explicitly verify `hBMDC` has a valid value and back out if it doesn't. I suggest using the braces even if not required. src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp line 190: > 188: VERIFY(::DeleteDC(hBMDC)); > 189: return; > 190: } The same goes here, `VERIFY(hBM != NULL);` is redundant, because the following `if` handles the condition. src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp line 191: > 189: return; > 190: } > 191: VERIFY(::GetDIBits(hBMDC, hBM, 0, 1, NULL, gpBitmapInfo, > DIB_RGB_COLORS)); I think the return value of `::GetDIBits` should also be analysed explicitly. I would prefer backing out if any of the functions above including `::GetDIBits` returns an error. Yet I'm not sure how critical it is to run the code below. [As I mentioned](https://github.com/openjdk/jdk/pull/17197#issuecomment-1875562325) in PR #17197, there's a workaround for a case where `::GetDIBits` returns 0. @MBaesken implemented a fallback to run the code below if even if all the above calls fail. It may be safe to ignore … in headless environment. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490702337 PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490694258 PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490700557 PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490697875 PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490736232 PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490704962 PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490709903 PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490712170 PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1490731331
