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

[Alexey Ivanov]

On Wed, 6 Mar 2024 07:49:35 GMT, Christoph Langer <clanger at openjdk.org> wrote:

> > It looks good to me.
> > The only question I have is for the fallback in `awt_Win32GraphicsDevice.cpp`.
> > Bailing out quickly makes the code cleaner. In this case, if `::GetDIBits` fails, we can bail out too.
> > If the fallback is needed, the code below the first call to `::GetDIBits` should be executed even if `GetDC` and `CreateCompatibleBitmap` fail.
> > Since testing didn't reveal any problems, the fallback is probably not as important, or not critical for a headless system.
> 
> I think the bailout is unlikely to happen after this fix. At least not for the headless environment where we won't jump into AwtWin32GraphicsDevice::Initialize() at all any more. And in headless environments with working monitors, we would never bail out, I guess. However, the method could be cleaned up I guess. But that should also be done in another issue and by somebody who is more into the details of what's going on there.

In this case, I think you should update the code to bail out if `::GetDIBits` fails. This way we will always bail out on any failure.

https://github.com/openjdk/jdk/blob/58a404443691b90439800fb2fd0f76b41667b707/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp#L192-L198

At the moment the code is inconsistent. If this code isn't even run in headless environment, then it's better to make it consistent. Otherwise, it raises questions why it returns if either if `GetDC` or `CreateCompatibleBitmap` fails but continues if `::GetDIBits` fails.

>> src/java.desktop/windows/classes/sun/awt/PlatformGraphicsInfo.java line 38:
>> 
>>> 36: 
>>> 37:     static {
>>> 38:         loadAWTLibrary();
>> 
>> Can `WToolkit.loadLibraries()` be used here? The method is declared public, so it should be accessible.
>> 
>> It may create a circular dependency though.
>
> Maybe. I think, as Phil already mentions, a cleanup/centralization of the places that load libawt should be done. But I would suggest to do this in a separate issue.

Sounds reasonable. Will you create a CR for this? So that we won't forget about it.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17614#issuecomment-1980364103
PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1514022885