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

[John Hendrikx]

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

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