Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from

Conversation

prsadhuk
Copy link
Collaborator

@prsadhuk prsadhuk commented Aug 22, 2023

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.


Progress

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

Issue

  • JDK-8262518: SwingNode.setContent does not close previous content, resulting in memory leak (Bug - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1219

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 22, 2023

👋 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 openjdk bot added the rfr Ready for review label Aug 22, 2023
@mlbridge
Copy link

mlbridge bot commented Aug 22, 2023

Webrevs

@hjohn
Copy link
Collaborator

hjohn commented Aug 22, 2023

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 Nodes 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.

@hjohn
Copy link
Collaborator

hjohn commented Aug 22, 2023

I should note, there is a memory leak still when using Disposer. Each time content is switched, but the SwingNode is re-used, a DisposerRecord is created that only gets cleaned up when SwingNode goes out of scope. In other words, a million content changes will leave behind a million DisposerRecords.

So, either there needs to be a removeRecord added to Disposer, or you could switch to the TreeShowingProperty solution.

@prsadhuk
Copy link
Collaborator Author

As suggested, optimized without WeakReference

Comment on lines 92 to 94
public static void removeRecord(DisposerRecord rec) {
disposerInstance.records.remove(rec);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but this won't work. The record is not the key, see this line:

         disposerInstance.records.put(ref, rec);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this removeRecord is necessary for swing-interop Disposer...Other javafx Disposer like
modules/javafx.base/src/main/java/com/sun/javafx/property/adapter/Disposer.java
also does not have removeRecord method

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this comment: #1219 (comment)

import com.sun.javafx.embed.swing.SwingNodeHelper;
import com.sun.javafx.embed.swing.SwingEvents;
import com.sun.javafx.embed.swing.newimpl.SwingNodeInteropN;
import jdk.swing.interop.LightweightFrameWrapper;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LightweightFrameWrapper import can be removed

@openjdk openjdk bot removed the rfr Ready for review label Aug 24, 2023
@openjdk openjdk bot added the rfr Ready for review label Aug 24, 2023
@andy-goryachev-oracle
Copy link
Contributor

/reviewers 2

@openjdk
Copy link

openjdk bot commented Aug 24, 2023

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with the code example in the ticker. Seems to work, the "Panels in memory" stays around 2, occasionally goes to 30-40 but then comes down to 2 pretty quickly. Same behavior even when Thread.sleep(1);

Questions:

  1. the JBS ticket refers to 3 other tickets - are we sure these are exact duplicates?
  2. the JBS talks about 1.8.0.271 - do we need to backport this fix to java8?

Comment on lines 92 to 95
/**
* Remove the DisposerRecord object to prevent memory leak
* @param ref Weak reference of the object to be removed
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, however I think you could say this a bit more general, no need to mention that it is to prevent memory leaks -- it's just normal that you should remove the record if there is no need for that specific dispose task anymore because it was already disposed in another way.

So how about:

Unregisters a previously registered {@link DisposerRecord}, removing it from the list of records to dispose off when the original target goes out of scope. This is useful in cases where the original target is still in use, but the record was already disposed off by other means.

Comment on lines 374 to 377
if (disposerRecRef != null) {
Disposer.removeRecord(disposerRecRef);
disposerRecRef = null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be part of the first if, or an if by itself, and not here. lwFrame, rec and disposerRecRef will all either be null or be set to some value. The only case this may not happen is that after lwFrame is set, an exception occurs. You could make it very safe, or just assume that when lwFrame is not null that all clean up actions should be taken -- if an exception did occur during set-up, then it's also fine that one can occur during clean-up, no need to make it more robust for situations that can't happen.

There really is no need to start second guessing yourself when the logic is solid. Either all those variables are non-null and checking one of them is sufficient, or they are all null and checking each in turn is pointless (it is just confusing because programmers may assume now that is a situation that can actually occur).

Please revert to the old code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hjohn I guess i have updated as per suggestion..Let me know if this is ok as per you..if yes, please approve..

@openjdk
Copy link

openjdk bot commented Aug 28, 2023

@prsadhuk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: angorya, jhendrikx

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

  • 53682dd: 8314589: javadoc build only shows the first 100 warnings and errors
  • be2c7ae: 8312058: Documentation improvements for subscription based listeners

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Aug 28, 2023
@kevinrushforth
Copy link
Member

Is it possible to write an automated test?

@prsadhuk
Copy link
Collaborator Author

/integrate

@openjdk
Copy link

openjdk bot commented Aug 29, 2023

Going to push as commit beca88c.
Since your change was applied there have been 2 commits pushed to the master branch:

  • 53682dd: 8314589: javadoc build only shows the first 100 warnings and errors
  • be2c7ae: 8312058: Documentation improvements for subscription based listeners

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 29, 2023
@openjdk openjdk bot closed this Aug 29, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Aug 29, 2023
@openjdk
Copy link

openjdk bot commented Aug 29, 2023

@prsadhuk Pushed as commit beca88c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@prsadhuk
Copy link
Collaborator Author

Is it possible to write an automated test?

oops..missed this..i can definitely try..maybe in upcoming test-sprint..

@prsadhuk prsadhuk deleted the JDK-8262518 branch August 29, 2023 04:17
@kevinrushforth
Copy link
Member

Can you file a follow-on test bug?

@prsadhuk
Copy link
Collaborator Author

Yes, JDK-8315317

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants