On Wed, 19 Nov 2025 12:52:51 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:
>
> Review comment
The fix now looks good with one question about the change in `EmbeddedScene`. I
also looked at the implementation of the `EmbeddedStage` and `EmbeddedScene`
methods, and they all do the right thing in that they forward the requests to
the FX thread.
I do want to see a follow-up issue filed to consider redesigning the threading
model, but I think this PR is a good workaround for the NPEs.
As for the newly added test, it passes with the fix and with no exception
messages (good). However, at least one of the times I ran the test without the
fix (using the existing 100 msec sleep), it printed a couple exceptions and
passed anyway. It seems that some of the exceptions that can occur will cause
the test to fail but others will not. You might consider using the
`test.javafx.util.OutputRedirect` utility instead of relying on the
`UncaughtExceptionHandler`. This might not be a problem in practice if you
reduce the sleep time, but will make the test more likely to catch any problems.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/EmbeddedScene.java
line 160:
> 158: if (getSceneState() != null) {
> 159: updateSceneState();
> 160: }
None of the other calls to `updateSceneState()` check for a null `sceneState`.
Why is this is the only one that needs to? If it is needed, it might be safer
to move the null check into `updateSceneState` itself.
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelNPETest.java line
103:
> 101: Thread.sleep(100);
> 102: Platform.runLater(() ->
> contentPane.setScene(webView.getScene()));
> 103: Thread.sleep(100);
With a `sleep(100)` this only occasionally gets an exception when I run it
without your fix. I recommend `sleep(1)` which is what the manual test does.
The test will take less time and also be a better stress test.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1968#pullrequestreview-3482862662
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2542150648
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2542248037
