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

[RealCLanger]

The assertions reported in the bug were observed spuriously and here and there broke tests in some Windows configurations.
For instance JDK-8266129, JDK-8269529 or 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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 (Bug - P3)

Reviewers

• Alexey Ivanov (@aivanov-jdk - Reviewer)
• Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17614/head:pull/17614
$ git checkout pull/17614

Update a local copy of the PR:
$ git checkout pull/17614
$ git pull https://git.openjdk.org/jdk.git pull/17614/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17614

View PR using the GUI difftool:
$ git pr show -t 17614

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17614.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back clanger! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@RealCLanger The following label will be automatically applied to this pull request:

• client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 10: Full - Incremental (ae139953)
• 09: Full - Incremental (2f378c88)
• 08: Full - Incremental (002fba2d)
• 07: Full - Incremental (7a00a647)
• 06: Full - Incremental (f43da52f)
• 05: Full - Incremental (61a78563)
• 04: Full - Incremental (aac66c9a)
• 03: Full - Incremental (86cc2b38)
• 02: Full - Incremental (58a40444)
• 01: Full - Incremental (ea8b6b2e)
• 00: Full (2a766183)

[MBaesken]

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.

This was my observation too, but is it always the case ? Unfortunately I couldn't find some documentation on this.

[MBaesken]

Maybe add some comment about the monitor counting, that it only considers now certain monitors with 'good' capabilities ?
Or even rename the count function and use a better name indicating what it does now ?

[RealCLanger]

I added a comment to explain that we skip monitors where CreateDC fails.

The fix runs successfully through SAP's testing with no regressions spotted. Looks like it even fixes the problems of test/jdk/java/awt/font/GlyphVector/LayoutCompatTest.java on Windows that we reported in the comments of https://bugs.openjdk.org/browse/JDK-8318364.

@prrace Want to take a look here (or assign somebody from client group)? Thanks!

[RealCLanger]

Hi @aivanov-jdk,

thanks for your suggestions. I addressed them. Please have another look.

[aivanov-jdk]

/reviewers 2

[openjdk]

@aivanov-jdk
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[aivanov-jdk]

Looks good to me…
except for comment on gpBitmapInfo in AwtWin32GraphicsDevice::Initialize.

[prrace]

This all looks reasonable initself but ..
The first thing I worry about here is that the obvious implication is that we can now have zero monitors.
Even if the monitor were not usable, that seems to have been "OK" in practice so long as we didn't assert out in a debug build.
Have you looked for any subsequent code that might break with zero monitors ?

The 2nd thing is that this situation ought to clearly flag that this is a HEADLESS situation and the AWT should be in headless mode.
And that decision usually needs to be made very early. So is this code path involved in that decision.
It seems likely there's a missing piece since I don't know that we even properly make that decision on windows like we do on Linux.

[RealCLanger]

The first thing I worry about here is that the obvious implication is that we can now have zero monitors. Even if the monitor were not usable, that seems to have been "OK" in practice so long as we didn't assert out in a debug build. Have you looked for any subsequent code that might break with zero monitors ?

I think that's hard to find out. At least our regression testing did not show issues. The change probably needs more testing. On the other hand, I think the risk with that change is low since the environment that is addressed by it seems to be rather unusual and the asserts show that something goes wrong in that case currently anyway.

The 2nd thing is that this situation ought to clearly flag that this is a HEADLESS situation and the AWT should be in headless mode. And that decision usually needs to be made very early. So is this code path involved in that decision. It seems likely there's a missing piece since I don't know that we even properly make that decision on windows like we do on Linux.

That's a valid point. We could flag a system where we detect zero monitors as headless and then set the headless property. At the moment the default headless property would be false on Windows in any case, as per here. I guess we can wire the monitor detection in here and return true if no monitors were found. Would you want me to go that way?

[prrace]

I think that's hard to find out. At least our regression testing did not show issues. The change probably needs more testing. On the other hand, I think the risk with that change is low since the environment that is addressed by it seems to be rather unusual and the asserts show that something goes wrong in that case currently anyway.

The 2nd thing is that this situation ought to clearly flag that this is a HEADLESS situation and the AWT should be in headless mode. And that decision usually needs to be made very early. So is this code path involved in that decision. It seems likely there's a missing piece since I don't know that we even properly make that decision on windows like we do on Linux.

That seems like a good idea. Definitely worth a shot to see if it is sufficient and in time !

That's a valid point. We could flag a system where we detect zero monitors as headless and then set the headless property. At the moment the default headless property would be false on Windows in any case, as per here. I guess we can wire the monitor detection in here and return true if no monitors were found. Would you want me to go that way?

[RealCLanger]

OK, I added code that would make GraphicsEnvironment::isHeadless() return true if we have no display devices. Please check 😄

[prrace]

The change looks good but I'd like to run it through all our tests since if there's anything wrong with this code path we are in trouble :-)

[RealCLanger]

The change looks good but I'd like to run it through all our tests since if there's anything wrong with this code path we are in trouble :-)

Yes, please test thoroughly and let me know. I'll give it another spin in our testing as well.

[RealCLanger]

OK, yes, seems I can reproduce. I'll try to figure out what's going on...

[RealCLanger]

I think I've understood the problem behind the crashes now. The way I implemented PlatformGraphicsInfo::getDefaultHeadlessProperty() modifies graphics initialization. I will change the implementation to just count the monitor number once for PlatformGraphicsInfo::getDefaultHeadlessProperty(), independently of when initDisplays() is running. Will post an update soon...

[prrace]

Does this new version really work for you ??

I'm getting
Execution failed: `main' threw exception: java.lang.UnsatisfiedLinkError: 'boolean sun.awt.PlatformGraphicsInfo.hasDisplays0()'

eg from
java\awt\Modal\ModalBlockingTests\UnblockedDialogAppModalTest.java

It is at least 840 tests that fail ..

Since I do see the method in your change .. and in my local copy too, I assume I correctly applied your patch
+Java_sun_awt_PlatformGraphicsInfo_hasDisplays0(JNIEnv *env, jclass thisClass) {

.. so I wonder if awt.dll is not loaded in time ? It isn't on demand.
It needs an explicit loadLibrary("awt") on all paths that can't rely on something else to do it.
What tests did you run and in what environment ?

[RealCLanger]

You're right, the awt lib needs to be loaded before checking isHeadless().

I just looked at the crash in test test/jdk/javax/swing/plaf/windows/6921687/bug6921687.java and resolved it. But the test obviously is not concerned about the headless environment so it doesn't stress that code path.

I reproduced the problem with a simple program that just calls GraphicsEnvironment.isHeadless() and fixed it. Please try the headless tests again.

Thanks a lot for your help!

[prrace]

This now passes all my automated testing.
Have you tested manual apps like SwingSet ?
I doubt they'll have a problem but it would be good to be 100% sure.

The only thing I notice here is it adds one more place that we duplicate the code to load the AWT native library. Bizarrely Toolkit and WToolkit both do it.
The result is not just duplicated code but duplicated (now triplicated) effort at startup.
There might even be a fourth place - NativeLibLoader.
But fixing that might mean being extra careful about not introducing circularities at start up and since the impact should be negligible I think it is out of scope for this fix.

So just let me know about the manual testing.

[aivanov-jdk]

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.

[RealCLanger]

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.

[RealCLanger]

This now passes all my automated testing. Have you tested manual apps like SwingSet ? I doubt they'll have a problem but it would be good to be 100% sure.

The only thing I notice here is it adds one more place that we duplicate the code to load the AWT native library. Bizarrely Toolkit and WToolkit both do it. The result is not just duplicated code but duplicated (now triplicated) effort at startup. There might even be a fourth place - NativeLibLoader. But fixing that might mean being extra careful about not introducing circularities at start up and since the impact should be negligible I think it is out of scope for this fix.

So just let me know about the manual testing.

I ran all of test/jdk/sanity/client/SwingSet now and everything passed. So from my end this should be ready.

[aivanov-jdk]

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.

jdk/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp

Lines 192 to 198 in 58a4044

	int getDiBitsRC = ::GetDIBits(hBMDC, hBM, 0, 1, NULL, gpBitmapInfo, DIB_RGB_COLORS);
	VERIFY(getDiBitsRC);
	if (getDiBitsRC == 0) {
	VERIFY(::DeleteObject(hBM));
	VERIFY(::DeleteDC(hBMDC));
	return;
	}

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.

[prrace]

I think we are done with this.

If a DEBUG build is used for HEADFUL then the new VERIFY calls might cause a different failure mode, but that's the only risk I see, and it seems a very, very small one.

[openjdk]

@RealCLanger This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: aivanov, prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 31 new commits pushed to the master branch:

• 5aae803: 8327390: JitTester: Implement temporary folder functionality
• 784f11c: 8327238: Remove MetadataAllocationFailALot* develop flags
• d7273ac: 8320646: RISC-V: C2 VectorCastHF2F
• 53c4714: 8327501: Common ForkJoinPool prevents class unloading in some cases
• 1261740: 8327283: RISC-V: Minimal build failed after JDK-8319716
• f54e598: 8327172: C2 SuperWord: data node in loop has no input in loop: replace assert with bailout
• 4018341: 8327379: Make TimeLinearScan a develop flag
• 1bd4abf: 8327434: Test java/util/PluggableLocale/TimeZoneNameProviderTest.java timed out
• ddcd6de: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
• 3d37b28: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
• ... and 21 more: https://git.openjdk.org/jdk/compare/3d106cb091de6b6ef2a9bf483fb0f5c98c28263c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[RealCLanger]

Thanks for the reviews.

/integrate

[openjdk]

Going to push as commit 1ad3ebc.
Since your change was applied there have been 116 commits pushed to the master branch:

• 7f6b7eb: 8327242: Document supported CLDR versions in the javadoc
• 8f9899b: 8325164: Named groups and signature schemes unavailable with SunPKCS11 in FIPS mode
• eb45d5b: 8327999: Remove copy of unused registers for cpu features check in x86_64 AVX2 Poly1305 implementation
• 5cae7d2: 8321299: runtime/logging/ClassLoadUnloadTest.java doesn't reliably trigger class unloading
• 35b00e6: 8327824: Type annotation placed on incorrect array nesting levels
• be344e4: 8327475: Add analysis code for JDK-8327169
• 0db6231: 8314508: Improve how relativized pointers are printed by frame::describe
• 0353245: 8325874: Improve checkbox-based interface in summary pages
• 4d64467: 8328079: JDK-8326583 broke ccache compilation
• 7e05a70: 8251330: Reorder CDS archived heap to speed up relocation
• ... and 106 more: https://git.openjdk.org/jdk/compare/3d106cb091de6b6ef2a9bf483fb0f5c98c28263c...master

Your commit was automatically rebased without conflicts.

[openjdk]

@RealCLanger Pushed as commit 1ad3ebc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.