On Tue, 10 Jun 2025 02:15:40 GMT, Anass Baya <ab...@openjdk.org> wrote:
>> 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. Do we actually need a new lock? can we reuse the AwtToolkit::GetInstance().InvokeFunction or AwtToolkit::GetInstance().SyncCall in a similar way it was done in https://github.com/openjdk/jdk/commit/d9bb0f0d39ed322b29c1807991e82fa28c4807f0? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25619#discussion_r2136828315
