On Tue, 10 Jun 2025 22:17:13 GMT, Anass Baya <ab...@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
>
> Anass Baya has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove global lock
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2505:
> 2503:
> 2504: // It's entirely possible that our native resources have been
> destroyed
> 2505: // before our java peer - if we're dispose()d, for instance.
You can merge this text and "// Return the default screen." to something like:
// Our native resources may have been destroyed before the Java peer,
// e.g., if dispose() was called. In that case, return the default screen.
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2507:
> 2505: // before our java peer - if we're dispose()d, for instance.
> 2506: // Return the default screen.
> 2507: JNI_CHECK_PEER_GOTO(self, ret);
I suggest reordering it slightly - this pattern is commonly used in most cases
where JNI_CHECK_PEER_GOTO is used:
result...;
AwtWindow *window = NULL;
PDATA pData;
JNI_CHECK_PEER_GOTO(self, ret);
window = (AwtWindow *)pData;
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25619#discussion_r2138935690
PR Review Comment: https://git.openjdk.org/jdk/pull/25619#discussion_r2138934388
