On Tue, 3 Jun 2025 19:05:06 GMT, Phil Race <p...@openjdk.org> wrote: >> Anass Baya has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix - 2 > > 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 ?
Hello @prrace, @mrserb, Thank you for your review. @prrace, regarding line 2521, perhaps they were returning 0 because they assumed the default device always has index 0, which is not true. I also agree with you — with proper synchronization, checking pData after verifying whether the peer is destroyed is unnecessary. @mrserb, yes — both situations can happen. For the second case (i.e. parallel execution), I verified it using a volatile flag. A race condition between the EDT and the AWT-Window thread is indeed possible. I didn’t add synchronization in the first fix to avoid impacting performance since I was not running into issues by just verfying if the peer is destroyed. However, the proper and complete fix would be: - Add synchronization between UnlinkObjects() (which, in this case, is invoked by the AWT-Window thread) and _GetScreenImOn() (which is called on the EDT thread). - Ensure that the peer is not destroyed inside _GetScreenImOn(). By doing both, we can be sure that no race condition occurs between _GetScreenImOn() and UnlinkObjects() that could corrupt pData, and we also protect _GetScreenImOn() from accessing a peer that has just been destroyed (as you assumed in your first point, @mrserb) I have added that proposal so you can take a look at it. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25619#discussion_r2136786438
