RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v2]

[Alexey Ivanov]

On Wed, 31 Jan 2024 07:23:13 GMT, Christoph Langer <clanger at 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