On Tue, 10 Jun 2025 03:07:55 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>> 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? Hello @mrserb, Yes, we can do that. I had the idea to use it so that UnlinkObjects() would be invoked by the EDT, which would eliminate the race condition. I didn’t proceed with it initially because I saw in that file that this pattern is only used in the JNI function calls, so I wanted to preserve the existing coding style paradigm. However, if you agree, I can move forward with this proposal. I actually prefer this approach, as it carries a lower risk of deadlocks and performance degradation as the codebase evolves and becomes more complex. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25619#discussion_r2136835388
