Skip to content
Permalink
Browse files
8208088: Memory Leak in ControlAcceleratorSupport
Reviewed-by: kcr, aghaisas
  • Loading branch information
arapte committed Apr 16, 2021
1 parent e8689fe commit 05ab7992a89cf9763528c14763ebc151c8f28613
@@ -27,6 +27,7 @@
import javafx.beans.InvalidationListener;
import javafx.beans.Observable;
import javafx.beans.property.ReadOnlyObjectProperty;
import javafx.beans.value.ChangeListener;
import javafx.collections.ListChangeListener;
import javafx.collections.ObservableList;
import javafx.event.Event;
@@ -46,6 +47,7 @@

import java.util.List;
import java.util.Map;
import java.util.WeakHashMap;

public class ControlAcceleratorSupport {

@@ -170,22 +172,32 @@ else if (menuitem instanceof CheckMenuItem) {

// We also listen to the accelerator property for changes, such
// that we can update the scene when a menu item accelerator changes.
menuitem.acceleratorProperty().addListener((observable, oldValue, newValue) -> {
final Map<KeyCombination, Runnable> accelerators = scene.getAccelerators();

// remove the old KeyCombination from the accelerators map
Runnable _acceleratorRunnable = accelerators.remove(oldValue);

// and put in the new accelerator KeyCombination, if it is not null
if (newValue != null) {
accelerators.put(newValue, _acceleratorRunnable);
}
});
menuitem.acceleratorProperty().addListener(getListener(scene, menuitem));
}
}
}

private static Map<MenuItem, ChangeListener<KeyCombination>> changeListenerMap = new WeakHashMap<>();

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

ChangeListener<KeyCombination> listener = changeListenerMap.get(menuItem);
if (listener == null) {
listener = (observable, oldValue, newValue) -> {
final Map<KeyCombination, Runnable> accelerators = scene.getAccelerators();

// remove the old KeyCombination from the accelerators map
Runnable _acceleratorRunnable = accelerators.remove(oldValue);

// and put in the new accelerator KeyCombination, if it is not null
if (newValue != null) {
accelerators.put(newValue, _acceleratorRunnable);
}
};
changeListenerMap.put(menuItem, listener);
}
return listener;
}

// --- Remove

@@ -220,7 +232,13 @@ public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, S

for (final MenuItem menuitem : items) {
if (menuitem instanceof Menu) {
// TODO remove the menu listener from the menu.items list
// MenuBarSkin uses MenuBarButton to display a Menu.
// The listener that is added on the 'items' in the method
// doAcceleratorInstall(final ObservableList<MenuItem> items, final Scene scene)
// is added to the MenuBarButton.getItems() and not to Menu.getItems().
// If a Menu is removed from scenegraph then it's skin gets disposed(), which disposes the
// related MenuBarButton. So it is not required to remove the listener that was added
// to MenuBarButton.getItems().

// remove the accelerators of items contained within the menu
removeAcceleratorsFromScene(((Menu)menuitem).getItems(), scene);
@@ -229,6 +247,12 @@ public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, S
// the scene accelerators map
final Map<KeyCombination, Runnable> accelerators = scene.getAccelerators();
accelerators.remove(menuitem.getAccelerator());

ChangeListener<KeyCombination> listener = changeListenerMap.get(menuitem);
if (listener != null) {
menuitem.acceleratorProperty().removeListener(listener);
changeListenerMap.remove(menuitem);
}
}
}
}
@@ -0,0 +1,101 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package test.javafx.scene.control;

import javafx.scene.control.MenuBar;
import javafx.scene.control.Menu;
import javafx.scene.control.MenuItem;
import javafx.scene.layout.BorderPane;

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

import org.junit.Test;
import org.junit.BeforeClass;
import static org.junit.Assert.assertEquals;

public class ControlAcceleratorSupportTest {

@Test
public void testNumberOfListenersByRemovingAndAddingMenuItems() {

Menu menu1 = new Menu("1");
MenuItem item11 = new MenuItem("Item 1");
MenuItem item12 = new MenuItem("Item 2");
menu1.getItems().addAll(item11, item12);

Menu menu2 = new Menu("2");
MenuItem item21 = new MenuItem("Item 1");
MenuItem item22 = new MenuItem("Item 2");
menu2.getItems().addAll(item21, item22);

MenuBar menuBar = new MenuBar();
menuBar.getMenus().addAll(menu1, menu2);
BorderPane pane = new BorderPane();
pane.setTop(menuBar);

StageLoader sl = new StageLoader(pane);

assertEquals(1, getListenerCount(item11.acceleratorProperty()));
assertEquals(1, getListenerCount(item12.acceleratorProperty()));
assertEquals(1, getListenerCount(item21.acceleratorProperty()));
assertEquals(1, getListenerCount(item22.acceleratorProperty()));

menu1.getItems().clear();
assertEquals(0, getListenerCount(item11.acceleratorProperty()));
assertEquals(0, getListenerCount(item12.acceleratorProperty()));
assertEquals(1, getListenerCount(item21.acceleratorProperty()));
assertEquals(1, getListenerCount(item22.acceleratorProperty()));

menu2.getItems().clear();
assertEquals(0, getListenerCount(item11.acceleratorProperty()));
assertEquals(0, getListenerCount(item12.acceleratorProperty()));
assertEquals(0, getListenerCount(item21.acceleratorProperty()));
assertEquals(0, getListenerCount(item22.acceleratorProperty()));

menu1.getItems().addAll(item11, item12);
assertEquals(1, getListenerCount(item11.acceleratorProperty()));
assertEquals(1, getListenerCount(item12.acceleratorProperty()));
assertEquals(0, getListenerCount(item21.acceleratorProperty()));
assertEquals(0, getListenerCount(item22.acceleratorProperty()));

menu2.getItems().addAll(item21, item22);
assertEquals(1, getListenerCount(item11.acceleratorProperty()));
assertEquals(1, getListenerCount(item12.acceleratorProperty()));
assertEquals(1, getListenerCount(item21.acceleratorProperty()));
assertEquals(1, getListenerCount(item22.acceleratorProperty()));

menu2.getItems().clear();
menu1.getItems().clear();

assertEquals(0, getListenerCount(item11.acceleratorProperty()));
assertEquals(0, getListenerCount(item12.acceleratorProperty()));
assertEquals(0, getListenerCount(item21.acceleratorProperty()));
assertEquals(0, getListenerCount(item22.acceleratorProperty()));

sl.dispose();
}
}

1 comment on commit 05ab799

@openjdk-notifier

This comment has been minimized.

Copy link

@openjdk-notifier openjdk-notifier bot commented on 05ab799 Apr 16, 2021

Please sign in to comment.