On Thu, 28 Dec 2023 09:48:52 GMT, Matthias Baesken <mbaes...@openjdk.org> wrote:
> When running with fastdebug binaries we run intermittent into the issue below
> in
> jtreg test
> java/awt/image/MultiResolutionImage/MultiResolutionImageObserverTest.java .
> Seems we miss checking of successful HBITMAP creation before calling
> GetDIBits.
>
> HDC hBMDC = this->GetDC();
> HBITMAP hBM = ::CreateCompatibleBitmap(hBMDC, 1, 1);
> VERIFY(::GetDIBits(hBMDC, hBM, 0, 1, NULL, gpBitmapInfo, DIB_RGB_COLORS));
>
> in awt_Win32GraphicsDevice.cpp . Thats why the releast of hBMDC / hBM fails
> as well at the end of the function causing the seond and third warning.
>
>
> Sat Nov 18 17:29:23 CET 2023
>
> *********************
> AWT Assertion Failure
> *********************
> ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0)
> File
> 'e:\openjdk\openjdk-21u-windows_x86_64-dbg\jdk\src\java.desktop\windows\native\libawt\windows\awt_Win32GraphicsDevice.cpp',
> at line 184
> GetLastError() is 57 : The parameter is incorrect.
>
> Do you want to break into the debugger?
> *********************
> *********************
> AWT Assertion Failure
> *********************
> ::DeleteObject(hBM)
> File
> 'e:\openjdk\openjdk-21u-windows_x86_64-dbg\jdk\src\java.desktop\windows\native\libawt\windows\awt_Win32GraphicsDevice.cpp',
> at line 297
> GetLastError() is 57 : The parameter is incorrect.
>
> So it seems, we should have some additional checks/tracing.
Is the purpose of this review to just add traces?
The errors seems to come from *headless* environment. A better fix would be to
add `@key headful` to the `MultiResolutionImageObserverTest.java` test, even
though the test does not display UI.
src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp line
142:
> 140: } else {
> 141: J2dTraceLn1(J2D_TRACE_WARNING,
> 142: "CreateDC failed, so we cannot store the DC for
> later usage, mieInfo.szDevice is %S",
Is `%S` a valid format string? It should in lower case: `%s`.
It seems, “store the DC for later usage” is misleading, it isn't stored… at
least for a long period of time.
src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp line
194:
> 192: if (hBM == NULL) {
> 193: J2dTraceLn(J2D_TRACE_WARNING,
> "AwtWin32GraphicsDevice::Initialize CreateCompatibleBitmap failed");
> 194: }
Does it make sense to call `::GetLastError()` and trace its value?
It seems that we cannot continue if `hBMDC` or `hBM` are null. Yet it seems
unexpected.
src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp line
309:
> 307: }
> 308: if (hBM != NULL) VERIFY(::DeleteObject(hBM));
> 309: if (hBMDC != NULL) VERIFY(::DeleteDC(hBMDC));
This should rather be expanded `if` statements with braces.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17197#pullrequestreview-1800910802
PR Review Comment: https://git.openjdk.org/jdk/pull/17197#discussion_r1439754392
PR Review Comment: https://git.openjdk.org/jdk/pull/17197#discussion_r1439757044
PR Review Comment: https://git.openjdk.org/jdk/pull/17197#discussion_r1439764934
