RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v7]

[Michael Strauß]

On Thu, 31 Aug 2023 20:13:40 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add nullcheck for sceneState
>
> The changes as they are now are IMHO entirely inadequate.  Simply surrounding all accesses to shared fields with a narrowly scoped `synchronized` block is not a magic solution. You will want the fields to be coherent for a longer period than that of a single field access.  For example:
> 
>         int lWidth = getCompWidth();
>         int lHeight = getCompHeight();
> 
> The new methods `getCompWidth` and `getCompHeight` are synchronized, but the overall code here is not.  This means that if the control resizes from `1000 x 1` to `1 x 1000` you may end up with a `lWidth` and `lWeight` of `1000 x 1000` or `1 x 1`.
> 
> IMHO the steps to resolve this to properly synchronized code is:
> 1. Find all external entry points (all non-private methods, and any private methods used by a listener callback)
> 2. For the entry points found above, mark which are called by FX and which by AWT
> 3. For the AWT methods note down all fields it accesses, including any in any methods it is calling
> 4. For the FX methods note down all fields it accesses, including any in any methods it is calling
> 5. Find the fields that are accessed in both the AWT and FX lists
> 6. Mark any methods that access those fields as `synchronized` -- this may be too course grained if these methods are large, do I/O, call back into other systems -- in that case you may need to move some code around to do all the code that needs synchronization first or last and use a `synchronized` block.
> 
> edit: further clarifications

I agree with @hjohn and will add one more comment: even if all local effects are taken into account, the code in this class might be racing against entirely unrelated code in the JavaFX framework. These races can't be reasonably addressed with local synchronization.

Coordinating multiple threads with potentially unknown side effects requires a bigger analysis effort, and the changes here do not inspire confidence that this has happened. I see no reason to replace the existing defective implementation with another potentially defective implementation.

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

PR Comment: https://git.openjdk.org/jfx/pull/1178#issuecomment-1701742095