RFR: 8262518: SwingNode.setContent does not close previous content, resulting in memory leak

[John Hendrikx]

On Tue, 22 Aug 2023 09:54:11 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> 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