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

[prsadhuk]

trafficstars

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)
• [x] Change must not contain extraneous whitespace
• [x] 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

[bridgekeeper[bot]]

:wave: 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[bot]]

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

[mlbridge[bot]]

Webrevs

• 02: Full - Incremental (2872159b)
• 01: Full - Incremental (6ddb5ab3)
• 00: Full (14ac6ef3)

[kevinrushforth]

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]

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

[andy-goryachev-oracle]

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

[prsadhuk]

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]

@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]

Check is not without precedence in this file..There are instance of assert host != null; and host != null check like below https://github.com/openjdk/jfx/blob/6a586b662592be3eb81670f0c5ce48061c2fc07c/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/EmbeddedScene.java#L185

[prsadhuk]

Automated test added

[andy-goryachev-oracle]

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.

[prsadhuk]

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]

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.

[bridgekeeper[bot]]

@prsadhuk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@prsadhuk This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.