similar to JDK-8293444: when re-using the a labeled's graphic somewhere else in a scenegraph (and also removing the labeled from the scenegraph), the labeled cannot be gc'ed.
The technical reason is a listener to a property of the graphic that is installed by the skin: it's manually removed on change of the graphics property and on dispose (sinceJDK-8247576) of the skin - none of which happens here.
The technical fix is to use a weak listener.
A deeper problem might be that now we have a labeled pointing to a graphic that's no longer contained in its own hierarchy. So if kept in the scenegraph, its skin will still react as if it were and f.i. compute the labeled's sizing based on the graphic.
My suggestion: fix the technical problem here (will do, since I overlooked the issue when fixing the leak on replacing the skin ;) and tackle the deeper problem in a follow-up.
Test for use in SkinLabeledCleanupTest, failing/passing before/after the suggested fix
@Test
public void testMoveGraphic() {
Node graphic = labeled.getGraphic();
WeakReference<Labeled> weakRef = new WeakReference<>(labeled);
root.getChildren().setAll(labeled);
stage.show();
root.getChildren().setAll(graphic);
Toolkit.getToolkit().firePulse();
labeled = null;
attemptGC(weakRef);
assertNull(weakRef.get());
}
Note: doing the same/similar inJDK-8293444 (for ScrollPane's listener on a property of its content) is probably not enough to fix the memory leak in that case because ScrollPaneSkin dispose is not yet implemented correctly (and it's still excluded from the cross-skin tests like SkinMemoryLeakTest). Didn't dig, but suspect that there's also a deeper problem as described above.
The technical reason is a listener to a property of the graphic that is installed by the skin: it's manually removed on change of the graphics property and on dispose (since
The technical fix is to use a weak listener.
A deeper problem might be that now we have a labeled pointing to a graphic that's no longer contained in its own hierarchy. So if kept in the scenegraph, its skin will still react as if it were and f.i. compute the labeled's sizing based on the graphic.
My suggestion: fix the technical problem here (will do, since I overlooked the issue when fixing the leak on replacing the skin ;) and tackle the deeper problem in a follow-up.
Test for use in SkinLabeledCleanupTest, failing/passing before/after the suggested fix
@Test
public void testMoveGraphic() {
Node graphic = labeled.getGraphic();
WeakReference<Labeled> weakRef = new WeakReference<>(labeled);
root.getChildren().setAll(labeled);
stage.show();
root.getChildren().setAll(graphic);
Toolkit.getToolkit().firePulse();
labeled = null;
attemptGC(weakRef);
assertNull(weakRef.get());
}
Note: doing the same/similar in
- relates to
-
JDK-8293444 Creating ScrollPane with same content component causes memory leak
- Resolved
-
JDK-8247576 Labeled/SkinBase: misbehavior on switching skin
- Resolved