RFR: 8346952 : GetGraphicsStressTest.java fails: Native resources unavailable

[Sergey Bylokhov]

On Tue, 3 Jun 2025 19:05:06 GMT, Phil Race <prr at openjdk.org> wrote:

>> **Analysis:**
>> 
>> We are encountering a race condition in the native code. While retrieving the screen number by calling _getScreenImOn(), the window is disposed. As a result, the AWT-Windows event loop processes the Dispose() call, which triggers UnlinkObjects().
>> The race condition between the execution paths of these two native methods sometimes causes an exception
>> 
>> **Proposed Fix:**
>> 
>> While it's possible to introduce a synchronization mechanism, it would not offer any real benefit. The window will be disposed regardless, and we’ll fall back to the default screen. This behavior is already handled in WWindowPeer.java, where a workaround is in place to use the default device when getScreenImOn() returns a non-existent screen number
>> 
>> 
>> public void updateGC() {
>> 
>>     int scrn = getScreenImOn();
>> 
>>     if (screenLog.isLoggable(PlatformLogger.Level.FINER)) {
>>         log.finer("Screen number: " + scrn);
>>     }
>> 
>>     // get current GD
>>     Win32GraphicsDevice oldDev = winGraphicsConfig.getDevice();
>> 
>>     Win32GraphicsDevice newDev;
>>     GraphicsDevice[] devs = GraphicsEnvironment
>>         .getLocalGraphicsEnvironment()
>>         .getScreenDevices();
>> 
>>     // Occasionally during device addition/removal getScreenImOn can return
>>     // a non-existing screen number. Use the default device in this case.
>>     if (scrn >= devs.length) {
>>         newDev = (Win32GraphicsDevice) GraphicsEnvironment
>>             .getLocalGraphicsEnvironment().getDefaultScreenDevice();
>>     } else {
>>         newDev = (Win32GraphicsDevice) devs[scrn];
>>     }
>> }
>> 
>> 
>> Therefore, I propose modifying the native method getScreenImOn to return the default device if the peer is being disposed :
>> 
>> jint AwtWindow::_GetScreenImOn(void *param)
>> {
>> ...
>>     jboolean destroyed = JNI_GET_DESTROYED(self);
>>     if (destroyed == JNI_TRUE){
>>         env->DeleteGlobalRef(self);
>>         return AwtWin32GraphicsDevice::GetDefaultDeviceIndex();
>>     }
>> ...
>> 
>> 
>> **Tests Summary:**
>> 
>>   GetGraphicsStressTest (existing test):
>> 
>>         Fails intermittently without the fix
>> 
>>         Consistently passes with the fix
>> 
>>   NotifyStressTest (newly added test):
>> 
>>          Consistently fails without the fix
>> 
>>          Consistently passes with the fix
>
> src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2515:
> 
>> 2513:         return AwtWin32GraphicsDevice::GetDefaultDeviceIndex();
>> 2514:     }
>> 2515: 
> 
> The code you referred to in the description is handling the case of when a *device* (ie screen) is disconnected. 
> This is the window peer being destroyed.
> Assuming that what you described is correct - some other thread is in a race with this thread - this change looks to me to greatly reduce the likelihood but not eliminate it - ie between the call at line 2510 above and 2516 below, the destruction could happen.
> 
> So isn't the right fix to not create the Throwable in this case ? And just do the default return.
> ie maybe the above check goes after the pData == NULL case and if destroyed, do the default return, and if its something else, do the throw.
> And perhaps (not sure) that line 2521 should always have returned the default device, not 0 ?

This looks suspicious that we might call these methods in the wrong order. But what's potentially even worse is if they are executed in parallel, can we destroy the peer at the exact moment another thread is working with it natively?
Is it similar to [this](https://github.com/openjdk/jdk/commit/cc944dde29f14b9bfa4d1437a55ad54f5523cd91) and [this](https://github.com/openjdk/jdk/commit/d9bb0f0d39ed322b29c1807991e82fa28c4807f0) bugs?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25619#discussion_r2126021940