On Thu, 20 Jul 2023 10:12:09 GMT, Prasanta Sadhukhan <psadhuk...@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
