RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer

[Michael Strauß]

On Tue, 18 Jul 2023 06:05:06 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.

The proposed patch does not appear to fix the issue. Here's how the code currently looks like (after the patch is applied):


786    @Override
787    protected void paintComponent(Graphics g) {
788        if (scenePeer == null) {
789            return;
790        }
791        if (pixelsIm == null) {
792            createResizePixelBuffer(scaleFactorX, scaleFactorY);
793            if (pixelsIm == null) {
794                return;
795            }
796        }
797        DataBufferInt dataBuf = (DataBufferInt)pixelsIm.getRaster().getDataBuffer();
798        int[] pixelsData = dataBuf.getData();
799        IntBuffer buf = IntBuffer.wrap(pixelsData);
800        if (scenePeer != null && !scenePeer.getPixels(buf, pWidth, pHeight)) {
801            // In this case we just render what we have so far in the buffer.
802        }


Note that in L788, we return early if `scenePeer` is null. The fact that `scenePeer` can sometimes be null in L800 means that the field is changed on a different thread. Since that's the case, no amount of null checking will ever be good enough: `scenePeer` can just as easily be null after the check succeeds in L800.

Similiar issues can be observed in other places, for example later in the same file:

    private class HostContainer implements HostInterface {
        @Override
        public void setEmbeddedScene(EmbeddedSceneInterface embeddedScene) {
            ...
            scenePeer = embeddedScene;
            ....
            invokeOnClientEDT(() -> {
                ....
                if (scenePeer != null) {
                    scenePeer.setDragStartListener(dnd.getDragStartListener());
                }
            });
        }
    }


Note that `scenePeer` is assigned on the thread that calls `setEmbeddedScene`, and then its value is read from the EDT.

Since it appears that `JFXPanel` is inherently multi-threaded, every usage of an unprotected shared field is potentially faulty. At the very least, every field that can be written to or read from multiple threads must be protected by a lock. Note that is is not enough to just protect all writes; all reads have to be protected as well.

In addition to that, we need to account for concurrent code execution. What happens if thread A runs code on `scenePeer`, while thread B has already created a new scene peer? Is this safe? If not, then we need to prevent such concurrent execution.

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

PR Comment: https://git.openjdk.org/jfx/pull/1178#issuecomment-1640402709