JDK-8216377: Fix memoryleak for initial nodes of Window #342
Conversation
The initial nodes of a Scene are not collected by the GC when they are removed from the Scene. They get collected when the initial Window is closed, but this is too late and not the expected behavior. This happens because the listener windowShowingChangedListener is not removed from the old Window, when the Scene is Changed. This commit makes sure, that this listener is properly removed. This bug would create a memoryleak for every node inside a window, if there wouldn't be another bug. The listener windowShowingChangedListener is only added to the Scene, when the window is changed. I'm pretty sure, this is not intentionally and also fixed with this commit. This might fix an unknown rendering related bug. This commit also contains an unit-test to make sure no regression will happen.
|
@FlorianKirmaier - Welcome to OpenJFX, and thank you for reporting this bug, and for proposing a solution. Before we can review it, we need a signed Oracle Contributor Agreement (OCA) on file from you. This can take several days, so if you have already sent one in, there is nothing else to do but wait until you hear back about your OCA. |
|
I've just signed the OCA. Now it probably just takes some time. |
In the mean time, I'll provide some quick feedback. |
tests/system/src/test/java/test/javafx/scene/InitialNodesMemoryLeakTest.java
Show resolved
Hide resolved
|
I've added a commit according to your feedback. I did only run the unit-tests provided in the openjfx project. |
There was a problem hiding this comment.
The fix looks correct to me. The test also looks good, and I verified that the test fails without the fix and passes with the fix. I left a couple small comments on the test.
I would like @arapte to review it as well, and run some more tests. If time, we can do this in parallel with your OCA being processed.
| public void start(Stage stage) throws Exception { | ||
| toCleanUp = new WeakReference<>(new Group()); | ||
|
|
||
| Group root = new Group(toCleanUp.get()); |
There was a problem hiding this comment.
There is a very small (but non-zero) window where the newly created child Group could be garbage collected between the time it goes out of scope (right after adding it as the referent of toCleanUp) and the call to toCleanUp().get. I recommend holding the new Group a local variable temporarily. Something like this:
Group group = new Group();
toCleanUp = new WeakReference<>(group);
Group root = new Group(group);
You could also set group = null after creating the root, if you want, but it isn't necessary, since group will go out of scope when start() returns, leaving the children list of root as the only hard reference to the object (until you clear it).
There was a problem hiding this comment.
That's a great Idea! I've changed the code accordingly.
tests/system/src/test/java/test/javafx/scene/InitialNodesMemoryLeakTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/InitialNodesMemoryLeakTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/InitialNodesMemoryLeakTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/InitialNodesMemoryLeakTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/InitialNodesMemoryLeakTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/InitialNodesMemoryLeakTest.java
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/InitialNodesMemoryLeakTest.java
Show resolved
Hide resolved
|
Thank you for the feedback! |
|
@FlorianKirmaier your OCA has been recorded. I will finish my review and merge the PR when done testing. Thank you for your contribution. |
|
Replying to a comment in the initial PR:
Indeed it does. I was able to confirm that your fix also fixes the following rending issue: JDK-8207837: Indeterminate ProgressBar does not animate if content is added after scene is set on window |
|
@kevinrushforth |
Not by default, no. In general we only backport critical bugs. It requires approval from a Project Lead (either @johanvos or myself) to get this backported to FX 11. In this instance, I would defer to Johan, so depending on what he says, you can send a formal request to openjfx-dev mailing list asking for approval to backport it. |
BugID: JDK-8216377
While working on an update for JPro, I've found out, that a unit test for a memory leak doesn't work on JavaFX11. It's a regression and doesn't happen on JavaFX8
The initial nodes of a Scene are not collected by the GC when they are removed from the Scene.
They get collected when the initial Window is closed, but this is too late and not the expected behavior.
Affected JavaFX versions: 9 10 11 develop
This happens because the listener windowShowingChangedListener is not removed from the old Window, when the Scene is Changed.
This commit makes sure, that this listener is properly removed.
This bug would create a memory leak for every node inside a window if there wouldn't be another bug.
The listener windowShowingChangedListener is only added to the Scene when the window is changed.
I'm pretty sure, this is not intentionally and also fixed with this commit.
This might fix an unknown rendering related bug.
This commit also contains an unit-test to make sure no regression will happen.