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
8241455: Memory leak on replacing selection/focusModel #148
Conversation
👋 Welcome back fastegal! A progress list of the required criteria for merging this PR into |
Webrevs
|
/reviewers 2 |
@kevinrushforth |
I have basically the same comment / question as I asked in #147 In general, there are two approaches to avoiding listener-related memory leaks. One is to use a WeakListener; the other is to explicitly remove the listener when the object is removed or otherwise no longer needed. Using a WeakListener is certainly easier, but runs the risk of the listener being removed too early and not cleaning up after itself. I'm not suggesting that's the case here, but it is worth looking at. |
I would like to widen the issue (and change this pull request accordingly) to include memory leaks of introduced selection- and focusModels in other controls that are caused by the basically same reason, that is strong references via listeners to controls' properties. The fix would be the same as well, that is wrap the listeners into weakXXListeners and register these. The models suggested to include here:
Not included would be memory leaks introduced by whacky listening of ChoiceBoxSkin and TabPaneSkin to properties of the selectionModel. The former will (most probably) be fixed as a side-effect of https://bugs.openjdk.java.net/browse/JDK-8087555 (which I intend to take, if Ajit agrees :), will open a separate issue for the latter. The extended fix here will have tests for replacing selection/focusModels with/out a skin registered for all controls having the respective models. Not entirely certain about the formal procedure (locally fix and tests are - nearly, pending some cleanup - ready). Simply change titles, rebase and push again? |
If Ambarish and Ajit are OK with this, then the procedure would be as you guessed: change the title of both the JBS bug and then this PR and push your new fix (with or without rebasing would be fine). |
That is good idea to combine all similarly affected selection models. I think rebase won't be required just updating the PR with more commits should be good enough. |
Please go ahead and take it up. Thanks! Regarding combining other issue. I am OK with the idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good to me.
Similar to Skin classes the listeners here are also not explicitly removed.
But I don't see any problematic scenario even if the listeners of an old Model are executed before it gets GCed.
Suggested minor changes in test.
...s/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java
Outdated
Show resolved
Hide resolved
...s/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java
Outdated
Show resolved
Hide resolved
...s/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java
Outdated
Show resolved
Hide resolved
stage = new Stage(); | ||
stage.setScene(scene); | ||
} | ||
root.getChildren().add(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be good to add a call to root.getChildren().removeAll()
before adding the node
to root
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be good to add a call to
root.getChildren().removeAll()
before adding thenode
toroot
?
hmm .. don't think that it's necessary to clear out other children: the thing that actually matters is that the control goes active in the scenegraph and has a skin attached. Can you think of any context where any of the tests might get whacky if there are more than one nodes in the root?
Actually, that's a pattern (*cough, read that's more or less c&p'ed) I use in other tests, where it sometimes is allowed and expected that there are more than one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that It would not cause any problem in test execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
@kleopatra This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type
Since the source branch of this PR was last updated there have been 10 commits pushed to the As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@arapte, @aghaisas) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
@kleopatra |
/sponsor |
@arapte @kleopatra The following commits have been pushed to master since your change was applied:
Your commit was automatically rebased without conflicts. Pushed as commit 560ef17. |
Several controls with selection/focusModels leave memory leaks on replacing the model.
Added tests for all such, those for the misbehaving models fail before and pass after the fix.
for convenience, the bug reference https://bugs.openjdk.java.net/browse/JDK-8241455
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jfx pull/148/head:pull/148
$ git checkout pull/148