8334593: Adding, removing and then adding a JFXPanel again leads to NullPointerException #1493

prsadhuk · 2024-06-28T08:49:49Z

Adding, then removing, and then adding a JFXPanel to the same component in the Swing scene graph leads to a NullPointerException in GlassScene because the sceneState is null.
Removing JFXPanel calls JFXPanel.removeNotify which calls Window.hide which calls SceneHelper.disposePeer -> Scene.disposePeer -> EmbeddedScene.dispose -> GlassScene.dispose which sets "sceneState" to null...
so when GlassScene.updateSceneState is called, it results in NPE.
Fix is to check if host (which is usually javafx.embed.swing.JFXPanel$HostContainer for active JFXPanel) has been reset/deleted which is done when EmbeddedScene.dispose is called during removeNotify and abstain from updating the scene state

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8334593: Adding, removing and then adding a JFXPanel again leads to NullPointerException (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1493/head:pull/1493
$ git checkout pull/1493

Update a local copy of the PR:
$ git checkout pull/1493
$ git pull https://git.openjdk.org/jfx.git pull/1493/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1493

View PR using the GUI difftool:
$ git pr show -t 1493

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1493.diff

Webrev

Link to Webrev Comment

…ullPointerException

bridgekeeper · 2024-06-28T08:50:35Z

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-06-28T08:51:13Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2024-06-28T08:54:32Z

Webrevs

kevinrushforth · 2024-06-28T14:12:37Z

Can you add an automated test to cover this fix?

Worth noting, this won't fully solve the problem that the reporter of this bug filed. The null check will prevent the NPE, but the scene will still not be visible. To allow removing and then re-adding the JFXPanel, we would need to not call Window.hide when the JFXPanel is removed. We should file a follow-on Enhancement to consider doing this, but that will need more discussion. The main point that would need to be solved is to figure out when to call Window.hide if not when the JFXPanel is removed.

kevinrushforth · 2024-06-28T15:08:10Z

@prsadhuk One more question: Do you know what changed in JavaFX 21 to trigger the NPE?

andy-goryachev-oracle · 2024-06-28T21:07:41Z

Since this problem straddles two different toolkits, I would suggest to specify a minimum of 2 reviewers.

prsadhuk · 2024-07-01T05:44:21Z

To allow removing and then re-adding the JFXPanel, we would need to not call Window.hide when the JFXPanel is removed.

I am not sure if this is a problem as just as JFXPanel.removeNotify does Window.hide but at the same time when we add JFXPanel to frame, it calls JFXPanel.addNotify which does Window.show so tit gets hidden when JFXPanel is removed and shown when JFXPanel is added

prsadhuk · 2024-07-01T05:46:26Z

@prsadhuk One more question: Do you know what changed in JavaFX 21 to trigger the NPE?

It is not reproducible in jfx21 as per my testing...The problematic code to call updateSceneState was added for JDK-8274932 so it has regressed from jfx22

prsadhuk · 2024-07-01T09:08:03Z

Check is not without precedence in this file..There are instance of assert host != null; and host != null check like below

jfx/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/EmbeddedScene.java

Line 185 in 6a586b6

if (host != null) {

prsadhuk · 2024-07-01T09:08:19Z

Automated test added

andy-goryachev-oracle · 2024-07-01T20:31:23Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/EmbeddedScene.java

@@ -157,7 +157,9 @@ public void setPixelScaleFactors(float scalex, float scaley) {
        entireSceneNeedsRepaint();
        Platform.runLater(() -> {
            QuantumToolkit.runWithRenderLock(() -> {
-                updateSceneState();
+                if (host != null) {


I think we need a different solution here, as mentioned in line 58

// TODO: synchronize access to embedder from ET and RT

andy-goryachev-oracle · 2024-07-01T20:43:44Z

Kevin is right, this fix does not solve the issue mentioned in the ticket. Once the fxPanel is added back, its content is not visible. Does not matter whether removing/adding happens at startup or the button event handler.

(attaching a slightly modified test case to the ticket, notice lines 30 and 44.

Also, I think swing requires validate() and repaint() called after modifying the component's children.

Maran23 · 2024-07-01T21:58:54Z

tests/system/src/test/java/test/javafx/embed/swing/JFXPanelNPETest.java

+                // fxPanel removed from frame
+                frame.remove(fxPanel);
+                // fxPanel added to frame again
+                frame.add(fxPanel); // <-- leads to NullPointerException


You can use JUnit5 to write this test (class) and use assertDoesNotThrow(() -> frame.add(fxPanel));, which makes this a bit better to understand

prsadhuk · 2024-07-02T03:45:24Z

Kevin is right, this fix does not solve the issue mentioned in the ticket. Once the fxPanel is added back, its content is not visible. Does not matter whether removing/adding happens at startup or the button event handler.

(attaching a slightly modified test case to the ticket, notice lines 30 and 44.

Also, I think swing requires validate() and repaint() called after modifying the component's children.

OK..I guess I probably misunderstood the expectation...it says

EXPECTED -
The JFXPanel is visible to the user of the application and no Exceptions are thrown.
ACTUAL -
The JFXPanel is visible but the following Exception is thrown

which I deciphered as the JFXPanel window being visible and I guess in original testcase execution, the "TestButton" was still visible, only NPE was thrown..

ANyway, I guess it was mentioned that "We should file a follow-on Enhancement to consider doing this" and regarding synchro mentioned at line58, I guess we already have QuantumToolkit.runWithRenderLock in the problematic code

andy-goryachev-oracle · 2024-07-02T17:13:56Z

QuantumToolkit.runWithRenderLock

locking a class field may not be sufficient. or rather, cannot be sufficient when multiple threads are accessing the field, as there are no guarantees as to the order of invocations.

Instead, we might need to pass the reference as a variable to the lambda which executes in a different thread. This way no matter what happens to the field, the code that executes in a different context will always have the right object.

And, it might be better not to share the field between the threads and toolkits. Always mutate a particular field in the context of one toolkit - like fieldAWT / fieldFX.

Just a suggestion.

8334593: Adding, removing and then adding a JFXPanel again leads to N…

14ac6ef

…ullPointerException

openjdk bot added the rfr Ready for review label Jun 28, 2024

Test added

6ebf5e8

openjdk bot removed the rfr Ready for review label Jul 1, 2024

Test added

6ddb5ab

openjdk bot added the rfr Ready for review label Jul 1, 2024

Test fix

2872159

andy-goryachev-oracle reviewed Jul 1, 2024

View reviewed changes

Maran23 reviewed Jul 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8334593: Adding, removing and then adding a JFXPanel again leads to NullPointerException #1493

8334593: Adding, removing and then adding a JFXPanel again leads to NullPointerException #1493

prsadhuk commented Jun 28, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 28, 2024

openjdk bot commented Jun 28, 2024

mlbridge bot commented Jun 28, 2024 •

edited

Loading

kevinrushforth commented Jun 28, 2024

kevinrushforth commented Jun 28, 2024

andy-goryachev-oracle commented Jun 28, 2024

prsadhuk commented Jul 1, 2024

prsadhuk commented Jul 1, 2024

prsadhuk commented Jul 1, 2024

prsadhuk commented Jul 1, 2024

andy-goryachev-oracle Jul 1, 2024

andy-goryachev-oracle commented Jul 1, 2024

Maran23 Jul 1, 2024

prsadhuk commented Jul 2, 2024

andy-goryachev-oracle commented Jul 2, 2024

8334593: Adding, removing and then adding a JFXPanel again leads to NullPointerException #1493

Are you sure you want to change the base?

8334593: Adding, removing and then adding a JFXPanel again leads to NullPointerException #1493

Conversation

prsadhuk commented Jun 28, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented Jun 28, 2024

openjdk bot commented Jun 28, 2024

mlbridge bot commented Jun 28, 2024 • edited Loading

Webrevs

kevinrushforth commented Jun 28, 2024

kevinrushforth commented Jun 28, 2024

andy-goryachev-oracle commented Jun 28, 2024

prsadhuk commented Jul 1, 2024

prsadhuk commented Jul 1, 2024

prsadhuk commented Jul 1, 2024

prsadhuk commented Jul 1, 2024

andy-goryachev-oracle Jul 1, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle commented Jul 1, 2024

Maran23 Jul 1, 2024

Choose a reason for hiding this comment

prsadhuk commented Jul 2, 2024

andy-goryachev-oracle commented Jul 2, 2024

prsadhuk commented Jun 28, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 28, 2024 •

edited

Loading