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
It's not entirely clear to me which thread is calling which methods in this
class, and it would help to clearly mark which part of the class can be called
from the FX thread and which parts from the AWT thread.
I also question how some of the other fields are used. Many are marked
`volatile` which is insufficient if you don't want to observe partial changes
of x/y coordinates for example.
I agree with @mstr2 that a more careful analysis couldn't hurt. There seem to
be only 2 threads involved, the AWT and FX Thread. The fields they are sharing
should be grouped and marked with a comment that they're shared. Any access to
these fields should then be only while holding a lock.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 152:
> 150:
> 151: private static final Object LOCK = new Object();
> 152:
I very much doubt it is the intention to synchronize on all JFXPanel instances,
so this should not be static.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 416:
> 414: }
> 415: return lScenePeer;
> 416: }
Suggestion:
private EmbeddedSceneInterface getScenePeer() {
synchronized(LOCK) {
return scenePeer;
}
}
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 424:
> 422: }
> 423: return lStagePeer;
> 424: }
Suggestion:
private EmbeddedStageInterface getStagePeer() {
synchronized(LOCK) {
return stagePeer;
}
}
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1041:
> 1039:
> 1040: @Override
> 1041: public void setEmbeddedStage(EmbeddedStageInterface
> embeddedStage) {
These changes don't inspire confidence in this solution. `stagePeer` is now no
longer updated when `embeddedStage` is `null`.
IMHO they should be:
@Override
public void setEmbeddedStage(EmbeddedStageInterface embeddedStage) {
synchronized(LOCK) {
stagePeer = embeddedStage;
if (stagePeer == null) {
return;
}
}
if (pWidth > 0 && pHeight > 0) {
embeddedStage.setSize(pWidth, pHeight);
}
invokeOnClientEDT(() -> {
if (JFXPanel.this.isFocusOwner()) {
embeddedStage.setFocused(true,
AbstractEvents.FOCUSEVENT_ACTIVATED);
}
});
sendMoveEventToFX();
}
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1069:
> 1067:
> 1068: @Override
> 1069: public void setEmbeddedScene(EmbeddedSceneInterface
> embeddedScene) {
Same comment as with `setEmbeddedStage`; surrounding all accesses to
`scenePeer` and `stagePeer` like this is not the way to do it, and looks
incorrect. I think this is more in the right direction:
public void setEmbeddedScene(EmbeddedSceneInterface embeddedScene) {
synchronized(LOCK) {
if (scenePeer == embeddedScene) {
return;
}
scenePeer = embeddedScene;
}
if (embeddedScene == null) {
invokeOnClientEDT(() -> {
if (dnd != null) {
dnd.removeNotify();
dnd = null;
}
});
return;
}
if (pWidth > 0 && pHeight > 0) {
embeddedScene.setSize(pWidth, pHeight);
}
embeddedScene.setPixelScaleFactors((float) scaleFactorX, (float)
scaleFactorY);
invokeOnClientEDT(() -> {
dnd = new SwingDnD(JFXPanel.this, scenePeer);
dnd.addNotify();
embeddedScene.setDragStartListener(dnd.getDragStartListener());
});
}
-------------
PR Review: https://git.openjdk.org/jfx/pull/1178#pullrequestreview-1541862971
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1271120246
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1271119728
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1271120451
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1271129358
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1271131346
