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

[Andy Goryachev]

On Thu, 20 Jul 2023 10:12:09 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:
> 
>   Check FXEnabled initially

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

> 1045:             }
> 1046:             lStagePeer = embeddedStage;
> 1047:             if (lStagePeer == null) {

there is something wrong with this code:
why we need a locked assignment on line 1044 that immediately gets overwritten on line 1046?

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

> 1090:             lScenePeer.setPixelScaleFactors((float) scaleFactorX, (float) scaleFactorY);
> 1091:             synchronized(LOCK) {
> 1092:                 scenePeer = lScenePeer;

I wonder if the whole method should be synchronized?
the logic around scenePeer is asking to be atomic, isn't it?

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

> 96:         });
> 97:         SwingUtilities.invokeAndWait(JFXPanelNPETest::createUI);
> 98:         for (int i = 0; i < 300; i++) {

is 300 sufficient?

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

> 99:             SwingUtilities.invokeLater(contentPane::repaint);
> 100:             Platform.runLater(() -> contentPane.setScene(null));
> 101:             Thread.sleep(100);

I would recommend replacing a fixed number with a random value.

(Typically, when a random is added to the test, it would make sense to print its seed to stdout, for the sake of reproducing a possible failure later; but in this case it might be unnecessary because we are dealing with multiple threads and that adds its own degree of randomness, completely eliminating reproducibility.  Or, perhaps, we still need to print the seed to maximize the probability of reproducing the failed scenario.)

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1270942415
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1270945833
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1270946674
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1270950218