On Tue, 18 Nov 2025 13:24:31 GMT, Prasanta Sadhukhan <[email protected]>
wrote:
>> NPE is seen while accessing transient "scenePeer" variable between reads..
>> Fix is made to store it in a temp variable rather than reading it twice
>> since the value can change between successive reads in many places it is
>> accessed.
>> Also some debug logs added to be enabled via `jfxpanel.debug` property
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> temp-var handling in few other methods
I left a few comments inline. I still want to look more closely at the
`sendXxxxxEventToFX()` methods to see if there are any problems using a stale
value of scenePeer() or stagePeer(). I do think this PR improves the situation
by avoiding spurious NPEs, so if further analysis doesn't show any additional
problems, it it probably reasonable to proceed with this PR and file a
follow-on bug to do a proper MT design for JFXPanel.
I also want do some additional testing.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 156:
> 154: private transient volatile EmbeddedWindow stage;
> 155:
> 156: private transient volatile Scene scene;
Minor: I would remove the blank line between these two declarations to make it
clear that the comment applies to both.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 974:
> 972: getScene().getFocusOwner() != null &&
> 973: getScene().getFocusOwner().isFocused()) {
> 974: hStagePeer.focusUngrab();
This can be reverted. `stagePeer` is only accessed on the FX thread.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 996:
> 994: // some reason they didn't install the grab
> when
> 995: // they were shown.
> 996: hStagePeer.focusUngrab();
This can be reverted. `stagePeer` is only accessed on the FX thread.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1023:
> 1021: }
> 1022: });
> 1023: sendMoveEventToFX();
This is not an equivalent change, since `sendMoveEventToFX()` can now happen
before `stage.show()` and will also happen unconditionally even if the stage is
null or already showing. Rather than moving it out of the runOnFxThread block,
leave it where it is and call `runOnEDT` like this:
SwingNodeHelper.runOnFxThread(() -> {
if ((stage != null) && !stage.isShowing()) {
stage.show();
SwingNodeHelper.runOnEDT(() -> sendMoveEventToFX());
}
});
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1078:
> 1076: stagePeer = embeddedStage;
> 1077: var hStagePeer = stagePeer;
> 1078: if (hStagePeer == null) {
If this is always invoked from the FX thread, you don't need to locally capture
the peers.
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelNPETest.java line
105:
> 103: Thread.sleep(100);
> 104: }
> 105: System.out.println("failure = " + failure.get());
This print can be removed.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1968#pullrequestreview-3478854934
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2539008637
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2539028674
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2539029530
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2539054147
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2539059009
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2539066037
