On Tue, 22 Aug 2023 09:54:11 GMT, Prasanta Sadhukhan <[email protected]>
wrote:
> Issue is when setting the content of a SwingNode, the old content is not
> garbage collected owing to the fact
> JLightweightFrame is never being released by SwingNodeDisposer
>
> The SwingNodeDisposer holds an hard pointer to the JLightweightFrame that
> prevents its collection
>
> Modified `SwingNode.setContentImpl` function to use a WeakReference to
> properly release the memory.
I think the solution is not optimal.
There are two references to the light weight frame:
1. SwingNode refers it as a field, which is changed on each content change so
no need to worry about that one
2. A `DisposerRecord` refers it to call `dispose` on the light weight frame.
`dispose` must be called in two situations:
1. When a frame is no longer needed because we're switching content
2. When `SwingNode` goes out of scope (as JavaFX has no "dispose" semantics for
`Node`s this requires a creative solution)
Case 1 is easy to solve, just dispose the light weight frame when content
changes. This is already done. What was missing here is that the disposer
record is not cleaned up as well (which also refers the frame), and which would
only get cleaned up when `SwingNode` goes out of scope (which it doesn't yet).
Case 2 has two solutions. One that uses weak references that aren't really
intended for resource management, and one that thinks about the lifecycle of
when the light weight frame is no longer needed.
1. Use a weak reference to `SwingNode` and when it goes out of scope call light
weight frame `dispose`
2. Check the showing status of `SwingNode` (other controls do this) and when it
is not showing, clean up resources (this happens immediately, and doesn't
require relying on the GC).
Now, the easiest solution to fix this without adding even more weak references
is to track the `DisposerRecord` and instead of calling `lwFrame.dispose` you
call `record.dispose`, like this:
private DisposerRecord rec;
/*
* Called on EDT
*/
private void setContentImpl(JComponent content) {
if (lwFrame != null) {
rec.dispose(); // disposes of the record's lwFrame pointer as well
as calling `dispose` on lwFrame
rec = null;
lwFrame = null;
}
if (content != null) {
lwFrame = swNodeIOP.createLightweightFrame();
SwingNodeWindowFocusListener snfListener =
new SwingNodeWindowFocusListener(this);
swNodeIOP.addWindowFocusListener(lwFrame, snfListener);
if (getScene() != null) {
Window window = getScene().getWindow();
if (window != null) {
swNodeIOP.notifyDisplayChanged(lwFrame,
window.getRenderScaleX(),
window.getRenderScaleY());
}
}
swNodeIOP.setContent(lwFrame,
swNodeIOP.createSwingNodeContent(content, this));
swNodeIOP.setVisible(lwFrame, true);
// track the record
rec = swNodeIOP.createSwingNodeDisposer(lwFrame);
Disposer.addRecord(this, rec);
if (getScene() != null) {
notifyNativeHandle(getScene().getWindow());
}
SwingNodeHelper.runOnFxThread(() -> {
locateLwFrame();// initialize location
if (focusedProperty().get()) {
activateLwFrame(true);
}
});
}
}
Now, the even better solution would be to get rid of `Disposer` completely.
This can be done by using a `TreeShowingProperty` which you create in the
constructor of `SwingNode`:
this.treeShowingProperty = new TreeShowingProperty(this);
By adding a `ChangeListener` to this property you get updates on whether the
`SwingNode` is currently showing or not. Note that when it goes out of scope,
it will **always** first be unshown (otherwise it can't go out of scope as
would still be referenced).
In this listener you can then call `dispose` when `showing` is `false`, or
create a new light weight frame when `showing` is `true`.
Examples of using the `TreeShowingProperty` that deal with similar issues can
be found in `PopupWindow` and `ProgressIndicatorSkin`. In both cases they need
to release resources so GC collection can succeed. `ProgressIndicatorSkin` has
to stop its animation (as the animation would otherwise keep a reference to the
skin), and `PopupWindow` needs to call `hide` to release native resources.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1219#issuecomment-1687987162
