FULL PRODUCT VERSION :
A DESCRIPTION OF THE PROBLEM :
When moving a MenuButton from one Scene to another (e.g. if the MenuButton is part of a draggable Tab that can be moved to another window), there is a memory leak. It turned out that there are 3 listeners that are always added but never removed.
The first is the scene property listener used to register the accelators: In the constructor of MenuButtonSkin, the listener on control.sceneProperty() always calls ControlAcceleratorSupport.addAcceleratorsIntoScene for the new scene, but not ControlAcceleratorSupport.removeAcceleratorsFromScene for the old scene. The following listener implementation fixed this aspect of the problem for me:
control.sceneProperty().addListener((scene, oldValue, newValue) -> {
if (getSkinnable() != null) {
if (oldValue != null) {
ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), oldValue);
}
if (newValue != null) {
ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), getSkinnable());
}
}
});
The second listener is the ListChangeListener on the menu items that is added in ControlAcceleratorSupport.doAcceleratorInstall(ObservableList<MenuItem>, Scene). A quick fix could be to make the method return the added listener thus making the caller able to unregister it when the scene changes. So the else block in ControlAcceleratorSupport.addAcceleratorsIntoScene(ObservableList<MenuItem>, Node) could be:
ListChangeListener<MenuItem> listener = doAcceleratorInstall(items, scene);
anchor.sceneProperty().addListener(new InvalidationListener() {
@Override public void invalidated(Observable observable) {
Scene scene = anchor.getScene();
if (scene == null) {
anchor.sceneProperty().removeListener(this);
items.removeListener(listener);
}
}
});
The third listener type are the accelerator property listeners registered in ControlAcceleratorSupport.doAcceleratorInstall(List<? extends MenuItem>, Scene). Fixing this might need some more changes as the current implementation assumes that the scene is always the same.
STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
1. Start the provided MenuButtonMemoryLeakTest. Do not yet press the "Test" button.
2. Run JVisualVM or a similar tool to create a heap dump of the running application
3. Press the Button "Test"
4. Create another heap dump and compare it to the previous dump. Check all instances with an instance count increase of 1000 (com.sun.javafx.scene.control.ControlAcceleratorSupport$$Lambda$*)
REPRODUCIBILITY :
This bug can be reproduced always.
---------- BEGIN SOURCE ----------
package test;
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.MenuButton;
import javafx.scene.control.MenuItem;
import javafx.scene.input.KeyCombination;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class MenuButtonMemoryLeakTest extends Application {
@Override
public void start(Stage stage) throws Exception {
VBox vbox = new VBox();
// create other VBox in another scene
VBox otherVbox = new VBox();
new Scene(otherVbox, 100, 100);
// create MenuButton having one item
MenuButton menuButton = new MenuButton();
MenuItem item = new MenuItem();
item.setAccelerator(KeyCombination.keyCombination("Ctrl+T"));
item.setOnAction(e -> System.out.println("Action"));
menuButton.getItems().add(item);
// add a button to move the MenuButton to the second stage
Button button = new Button("Test");
button.setOnAction(e -> {
// move MenuButton from one scene to another many times
for (int i = 0; i < 1000; i++) {
if (vbox.getChildren().contains(menuButton)) {
vbox.getChildren().remove(menuButton);
item.setAccelerator(KeyCombination.keyCombination("Ctrl+S"));
otherVbox.getChildren().add(menuButton);
} else {
otherVbox.getChildren().remove(menuButton);
vbox.getChildren().add(menuButton);
}
}
});
vbox.getChildren().addAll(button, menuButton);
Scene scene = new Scene(vbox, 100, 100);
stage.setScene(scene);
stage.show();
}
public static void main(String[] args) throws Exception {
Application.launch(args);
}
}
---------- END SOURCE ----------
A DESCRIPTION OF THE PROBLEM :
When moving a MenuButton from one Scene to another (e.g. if the MenuButton is part of a draggable Tab that can be moved to another window), there is a memory leak. It turned out that there are 3 listeners that are always added but never removed.
The first is the scene property listener used to register the accelators: In the constructor of MenuButtonSkin, the listener on control.sceneProperty() always calls ControlAcceleratorSupport.addAcceleratorsIntoScene for the new scene, but not ControlAcceleratorSupport.removeAcceleratorsFromScene for the old scene. The following listener implementation fixed this aspect of the problem for me:
control.sceneProperty().addListener((scene, oldValue, newValue) -> {
if (getSkinnable() != null) {
if (oldValue != null) {
ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), oldValue);
}
if (newValue != null) {
ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), getSkinnable());
}
}
});
The second listener is the ListChangeListener on the menu items that is added in ControlAcceleratorSupport.doAcceleratorInstall(ObservableList<MenuItem>, Scene). A quick fix could be to make the method return the added listener thus making the caller able to unregister it when the scene changes. So the else block in ControlAcceleratorSupport.addAcceleratorsIntoScene(ObservableList<MenuItem>, Node) could be:
ListChangeListener<MenuItem> listener = doAcceleratorInstall(items, scene);
anchor.sceneProperty().addListener(new InvalidationListener() {
@Override public void invalidated(Observable observable) {
Scene scene = anchor.getScene();
if (scene == null) {
anchor.sceneProperty().removeListener(this);
items.removeListener(listener);
}
}
});
The third listener type are the accelerator property listeners registered in ControlAcceleratorSupport.doAcceleratorInstall(List<? extends MenuItem>, Scene). Fixing this might need some more changes as the current implementation assumes that the scene is always the same.
STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
1. Start the provided MenuButtonMemoryLeakTest. Do not yet press the "Test" button.
2. Run JVisualVM or a similar tool to create a heap dump of the running application
3. Press the Button "Test"
4. Create another heap dump and compare it to the previous dump. Check all instances with an instance count increase of 1000 (com.sun.javafx.scene.control.ControlAcceleratorSupport$$Lambda$*)
REPRODUCIBILITY :
This bug can be reproduced always.
---------- BEGIN SOURCE ----------
package test;
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.MenuButton;
import javafx.scene.control.MenuItem;
import javafx.scene.input.KeyCombination;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class MenuButtonMemoryLeakTest extends Application {
@Override
public void start(Stage stage) throws Exception {
VBox vbox = new VBox();
// create other VBox in another scene
VBox otherVbox = new VBox();
new Scene(otherVbox, 100, 100);
// create MenuButton having one item
MenuButton menuButton = new MenuButton();
MenuItem item = new MenuItem();
item.setAccelerator(KeyCombination.keyCombination("Ctrl+T"));
item.setOnAction(e -> System.out.println("Action"));
menuButton.getItems().add(item);
// add a button to move the MenuButton to the second stage
Button button = new Button("Test");
button.setOnAction(e -> {
// move MenuButton from one scene to another many times
for (int i = 0; i < 1000; i++) {
if (vbox.getChildren().contains(menuButton)) {
vbox.getChildren().remove(menuButton);
item.setAccelerator(KeyCombination.keyCombination("Ctrl+S"));
otherVbox.getChildren().add(menuButton);
} else {
otherVbox.getChildren().remove(menuButton);
vbox.getChildren().add(menuButton);
}
}
});
vbox.getChildren().addAll(button, menuButton);
Scene scene = new Scene(vbox, 100, 100);
stage.setScene(scene);
stage.show();
}
public static void main(String[] args) throws Exception {
Application.launch(args);
}
}
---------- END SOURCE ----------
- relates to
-
JDK-8175358 Memory leak when moving MenuButton into another Scene
- Resolved