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