[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: RFR: 8280468: Crashes in getConfigColormap, getConfigVisualId, XVisualIDFromVisual on Linux
From:       Sergey Bylokhov <serb () openjdk ! java ! net>
Date:       2022-03-29 23:16:52
Message-ID: STZ-wV5dpSvCLD1MyOtYk7VhJoKUxVOl4_v_bKjxoCI=.75ec4331-12c7-4a08-96bd-e700f3e59fd3 () github ! com
[Download RAW message or body]

On Fri, 21 Jan 2022 17:02:38 GMT, Maxim Kartashev <duke@openjdk.java.net> wrote:

> These crashes were not reproducible, so the fix is based on a hypothesis that there \
> are two possible reasons for them: 1. `makeDefaultConfig()` returning `NULL`.
> 2. A race condition when the number of screens changes.
> The race scenario: `X11GraphisDevice.makeDefaultConfiguration()` is called on EDT \
> so the call can race with `X11GraphisDevice.invalidate()` that re-sets the screen \
> number of the device; the latter is invoked on the `AWT-XAWT` thread from \
> `X11GraphicsEnvironment.rebuildDevices()`. So by the time \
> `makeDefaultConfiguration()` makes a native call with the device's current screen \
> number, the `x11Screens` array maintained by the native code could have become \
> shorter. And the native methods like \
> `Java_sun_awt_X11GraphicsDevice_getConfigColormap()` assume that the number passed \
> to them is always current and valid. The AWT lock only protects against the changes \
> during the native methods invocation and does not protect against them being called \
> with an outdated screen number. With a larger screen number, those methods read \
> past the end of the `x11Screens` array. 
> The fix for (1) is to die gracefully instead of crashing in an attempt to \
> de-reference a `NULL` pointer, which might happen upon returning from \
> `makeDefaultConfig()` in `getAllConfigs()`. 
> The fix for (2) is to eliminate the race by protecting `X11GraphisDevice.screen` \
> with the AWT lock such that it doesn't change when the native methods working with \
> it are active. 
> We've been shipping JetBrains Runtime with this fix for a few months now and there \
> were no crash reports with those specific patterns against the versions with the \
> fix.

I think we can update the fix by these suggestions above:

- Can we remove the `X11GraphicsDevice.configLock` from the `X11GraphicsDevice` and \
use awt lock instead? I am not sure can we get a deadlock for the current code where \
we will have `X11GraphicsDevice.configLock` -> `awt lock` in the `X11GraphicsDevice`, \
and `awt lock` -> `X11GraphicsDevice.configLock` in the `XCanvasPeer. \
getAppropriateGraphicsConfiguration` (Or in some other similar code)

- The code in the `XCanvasPeer` most probably can be rewritten w/o using the \
screenNum and the internal API, by the iteration over the newDev.getConfigurations() \
and check the visual of each config. In this case, we will reuse the locks added to \
the  `X11GraphicsDevice`

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

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


[prev in list] [next in list] [prev in thread] [next in thread]