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

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v7]
From:       Michael =?UTF-8?B?U3RyYXXDnw==?= <mstrauss () openjdk ! org>
Date:       2023-08-31 20:35:10
Message-ID: wFpUxwHvAtZTxseU1H5TyYpXOCWmgEtvp3sltHcUwJM=.162c603a-a4b7-4b8d-890e-9c653bcf1bfd () github ! com
[Download RAW message or body]

On Thu, 31 Aug 2023 20:13:40 GMT, John Hendrikx <jhendrikx@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


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