Skip to content

Commit

Permalink
8274022: Additional Memory Leak in ControlAcceleratorSupport
Browse files Browse the repository at this point in the history
Reviewed-by: mstrauss, kcr
  • Loading branch information
FlorianKirmaier authored and kevinrushforth committed Nov 15, 2021
1 parent 7ddd642 commit 0d5b8f8
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 13 deletions.
Expand Up @@ -45,6 +45,7 @@
import javafx.scene.control.TreeTableColumn;
import javafx.scene.input.KeyCombination;

import java.lang.ref.WeakReference;
import java.util.List;
import java.util.Map;
import java.util.WeakHashMap;
Expand Down Expand Up @@ -82,8 +83,12 @@ public static void addAcceleratorsIntoScene(ObservableList<MenuItem> items, Node
// 1. Installing accelerators when Control is added to Scene
// 2. Removing accelerators when Control is removed from Scene
// Remove previously added listener if any
if (sceneChangeListenerMap.containsKey(anchor)) {
anchor.sceneProperty().removeListener(sceneChangeListenerMap.get(anchor));
WeakReference<ChangeListener<Scene>> listenerW = sceneChangeListenerMap.get(anchor);
if (listenerW != null) {
ChangeListener<Scene> listener = listenerW.get();
if (listener != null) {
anchor.sceneProperty().removeListener(listener);
}
sceneChangeListenerMap.remove(anchor);
}
// Add a new listener
Expand Down Expand Up @@ -113,10 +118,12 @@ private static void addAcceleratorsIntoScene(ObservableList<MenuItem> items, Obj
}
}

private static Map<Object, ChangeListener<Scene>> sceneChangeListenerMap = new WeakHashMap<>();
/* It's okay to have the value Weak, because we only remember it to remove the listener later on */
private static Map<Object, WeakReference<ChangeListener<Scene>>> sceneChangeListenerMap = new WeakHashMap<>();

private static ChangeListener<Scene> getSceneChangeListener(Object anchor, ObservableList<MenuItem> items) {
ChangeListener<Scene> sceneChangeListener = sceneChangeListenerMap.get(anchor);
WeakReference<ChangeListener<Scene>> sceneChangeListenerW = sceneChangeListenerMap.get(anchor);
ChangeListener<Scene> sceneChangeListener = sceneChangeListenerW == null ? null : sceneChangeListenerW.get();
if (sceneChangeListener == null) {
sceneChangeListener = (ov, oldScene, newScene) -> {
if (oldScene != null) {
Expand All @@ -126,7 +133,7 @@ private static ChangeListener<Scene> getSceneChangeListener(Object anchor, Obser
doAcceleratorInstall(items, newScene);
}
};
sceneChangeListenerMap.put(anchor, sceneChangeListener);
sceneChangeListenerMap.put(anchor, new WeakReference<>(sceneChangeListener));
}
return sceneChangeListener;
}
Expand Down Expand Up @@ -193,11 +200,13 @@ else if (menuitem instanceof CheckMenuItem) {
}
}

private static Map<MenuItem, ChangeListener<KeyCombination>> changeListenerMap = new WeakHashMap<>();
/* It's okay to have the value Weak, because we only remember it to remove the listener later on */
private static Map<MenuItem, WeakReference<ChangeListener<KeyCombination>>> changeListenerMap = new WeakHashMap<>();

private static ChangeListener<KeyCombination> getListener(final Scene scene, MenuItem menuItem) {

ChangeListener<KeyCombination> listener = changeListenerMap.get(menuItem);
WeakReference<ChangeListener<KeyCombination>> listenerW = changeListenerMap.get(menuItem);
ChangeListener<KeyCombination> listener = listenerW == null ? null : listenerW.get();
if (listener == null) {
listener = (observable, oldValue, newValue) -> {
final Map<KeyCombination, Runnable> accelerators = scene.getAccelerators();
Expand All @@ -210,7 +219,7 @@ private static ChangeListener<KeyCombination> getListener(final Scene scene, Men
accelerators.put(newValue, _acceleratorRunnable);
}
};
changeListenerMap.put(menuItem, listener);
changeListenerMap.put(menuItem, new WeakReference<>(listener));
}
return listener;
}
Expand Down Expand Up @@ -241,8 +250,12 @@ public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, N
if (scene == null) {
// The Node is not part of a Scene: Remove the Scene listener that was added
// at the time of installing the accelerators.
if (sceneChangeListenerMap.containsKey(anchor)) {
anchor.sceneProperty().removeListener(sceneChangeListenerMap.get(anchor));
WeakReference<ChangeListener<Scene>> listenerW = sceneChangeListenerMap.get(anchor);
if (listenerW != null) {
ChangeListener<Scene> listener = listenerW.get();
if (listener != null) {
anchor.sceneProperty().removeListener(listener);
}
sceneChangeListenerMap.remove(anchor);
}
}
Expand Down Expand Up @@ -272,9 +285,12 @@ public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, S
final Map<KeyCombination, Runnable> accelerators = scene.getAccelerators();
accelerators.remove(menuitem.getAccelerator());

ChangeListener<KeyCombination> listener = changeListenerMap.get(menuitem);
if (listener != null) {
menuitem.acceleratorProperty().removeListener(listener);
WeakReference<ChangeListener<KeyCombination>> listenerW = changeListenerMap.get(menuitem);
if (listenerW != null) {
ChangeListener<KeyCombination> listener = listenerW.get();
if (listener != null) {
menuitem.acceleratorProperty().removeListener(listener);
}
changeListenerMap.remove(menuitem);
}
}
Expand Down
Expand Up @@ -29,8 +29,10 @@
import javafx.scene.control.Menu;
import javafx.scene.control.MenuItem;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.StackPane;

import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
import test.util.memory.JMemoryBuddy;
import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;

import org.junit.Test;
Expand Down Expand Up @@ -98,4 +100,19 @@ public void testNumberOfListenersByRemovingAndAddingMenuItems() {

sl.dispose();
}

@Test
public void testMemoryLeak_JDK_8274022() {
JMemoryBuddy.memoryTest(checker -> {
MenuItem menuItem = new MenuItem("LeakingItem");
MenuBar menuBar = new MenuBar(new Menu("MENU_BAR", null, menuItem));
StageLoader sl = new StageLoader(new StackPane(menuBar));
sl.getStage().close();

// Set listener to something on the scene, to make sure the listener references the whole scene.
menuItem.setOnAction((e) -> { menuItem.fire();});

checker.assertCollectable(menuItem);
});
}
}

1 comment on commit 0d5b8f8

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.