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

[Andy Goryachev]

On Wed, 30 Aug 2023 06:49:52 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Due to transient datatype of scenePeer, it can become null which can result in NPE in scenarios where scene is continuously been reset and set, which warrants a null check, as is done in other places for the same variable.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add nullcheck for sceneState

overall looks good - the new LOCK'ing logic appears I have some minor comments

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 444:

> 442:             return pHeight;
> 443:         }
> 444:     }

perhaps we could add setCompSize(int, int) to set both pHeight and pWidth at the same time, in order to avoid multiple LOCK'ed blocks down below (L638, L1068)?

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 450:

> 448:             return scaleFactorX;
> 449:         }
> 450:     }

same idea, possibly add setScaleFactor(double, double)?

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 885:

> 883:         double lScaleFactorX = getScaleFactorX();
> 884:         double lScaleFactorY = getScaleFactorY();
> 885:         if (lScenePeer == null) {

minor: move check on L885 right after L880, no need to lock 4 times if lScenePeer is null

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1147:

> 1145:             invokeOnClientEDT(() -> {
> 1146:                 SwingDnD lDnD = getDnD();
> 1147:                 lDnD = new SwingDnD(JFXPanel.this, embeddedScene);

is there a need for getting getDnd() on L1146 and immediately overwriting it on L1147?
should it be
`SwingDnD lDnD = new SwingDnD(JFXPanel.this, embeddedScene);`
?

tests/system/src/test/java/test/javafx/embed/swing/JFXPanelNPETest.java line 44:

> 42: import org.junit.Assert;
> 43: import org.junit.BeforeClass;
> 44: import org.junit.Test;

should we be using junit5?

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

PR Review: https://git.openjdk.org/jfx/pull/1178#pullrequestreview-1605556486
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312121220
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312122175
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312128303
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312132172
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312132992