RFR: 8280468: Crashes in getConfigColormap, getConfigVisualId, XVisualIDFromVisual on Linux

[Sergey Bylokhov]

On Mon, 24 Jan 2022 18:45:50 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:

>> Thanks for looking at this!
>> 
>>> You can try to reproduce the bug by creating the custom JDK and emulating the native upcalls by the test. It will help to prve that the fix will work.
>> 
>> Can you elaborate, please? Do you mean that this custom JDK would synthetically change `X11GraphicsDevice.screen` at the "right" time?
>> 
>>> 
>>> I am not sure that it is possible to synchronize access to the screen number across the code w/o introducing deadlocks, the current change does not cover all cases where the X11GraphicsDevice.screen is passed around. probably we should follow this [suggestion](https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.desktop/unix/classes/sun/awt/X11GraphicsDevice.java#L58) and everywhere the "screen" is used under the awt lock (1)check that it is less than the number of screens and use 0(main screen as a fallback), or (2) some "default value" if the number of screens is zero at that time - something similar was implemented on macOS by the [JDK-8211992](http://hg.openjdk.java.net/jdk/jdk/rev/814c49afb1a7)
>> 
>> I agree, returning some default as a fallback should also work. However, that's so radically different from the approach I've taken that a separate PR would be required.
>
>> Can you elaborate, please? Do you mean that this custom JDK would synthetically change `X11GraphicsDevice.screen` at the "right" time?
> 
> Yes, at the "right" time and the "right" number of monitors.
> 
>> > I am not sure that it is possible to synchronize access to the screen number across the code w/o introducing deadlocks, the current change does not cover all cases where the X11GraphicsDevice.screen is passed around. probably we should follow this [suggestion](https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.desktop/unix/classes/sun/awt/X11GraphicsDevice.java#L58) and everywhere the "screen" is used under the awt lock (1)check that it is less than the number of screens and use 0(main screen as a fallback), or (2) some "default value" if the number of screens is zero at that time - something similar was implemented on macOS by the [JDK-8211992](http://hg.openjdk.java.net/jdk/jdk/rev/814c49afb1a7)
>> 
>> I agree, returning some default as a fallback should also work. However, that's so radically different from the approach I've taken that a separate PR would be required.
> 
> It is better to start from the bug reproducing. As the change - the awt lock always should be the last one, I am not sure that wrapping a bunch of code by that lock is safe.

> 2. The idea of testing if the fix actually cures the crash suggested by @mrserb will work, but only to prove that one particular hypothesis about the crash is correct. 

Even this confirmation will be useful.

> But our our users kind of proved that already and I'm very reluctant to replaces what I believe is already working with what will probably work.

* If we will go this way then we will need to synchronize all access to the "screen" field of the X11GraphicsDevice by the awt lock. The current fix does not cover the case when the screen field is leaked outside the class via X11GraphicsDevice.getScreen() method. Can that "external code" also crash?
* We probably should remove the "X11GraphicsDevice.configLock" lock from the X11GraphicsDevice and use awt lock instead.
* The code in the XCanvasPeer most probably can be rewritten w/o using the screenNum, by the iteration over the newDev.getConfigurations() and check the visual of each config.

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

PR: https://git.openjdk.java.net/jdk/pull/7182