Skip to content

Commit

Permalink
8283551: ControlAcceleratorSupport menu items listener causes memory …
Browse files Browse the repository at this point in the history
…leak

Reviewed-by: angorya, mstrauss
  • Loading branch information
Dean Wookey authored and arapte committed Jun 28, 2023
1 parent 8b1a446 commit 8fd2943
Showing 4 changed files with 425 additions and 20 deletions.
Original file line number Diff line number Diff line change
@@ -50,6 +50,7 @@ public class ExpressionHelperUtility {
private static final String LIST_EXPRESSION_HELPER_SINGLE_CHANGE = "com.sun.javafx.binding.ListExpressionHelper$SingleChange";
private static final String LIST_EXPRESSION_HELPER_SINGLE_LIST_CHANGE = "com.sun.javafx.binding.ListExpressionHelper$SingleListChange";
private static final String LIST_EXPRESSION_HELPER_GENERIC = "com.sun.javafx.binding.ListExpressionHelper$Generic";
private static final String LIST_LISTENER_HELPER_GENERIC = "com.sun.javafx.collections.ListListenerHelper$Generic";
private static final String MAP_EXPRESSION_HELPER_SINGLE_INVALIDATION = "com.sun.javafx.binding.MapExpressionHelper$SingleInvalidation";
private static final String MAP_EXPRESSION_HELPER_SINGLE_CHANGE = "com.sun.javafx.binding.MapExpressionHelper$SingleChange";
private static final String MAP_EXPRESSION_HELPER_SINGLE_MAP_CHANGE = "com.sun.javafx.binding.MapExpressionHelper$SingleMapChange";
@@ -199,9 +200,12 @@ public static <T> List<ChangeListener<? super T>> getChangeListeners(ObservableV
}

public static <E> List<ListChangeListener<? super E>> getListChangeListeners(ObservableList<E> observable) {
final Object helper = getExpressionHelper(observable);
Object helper = getExpressionHelper(observable);
if (helper == null) {
return Collections.emptyList();
helper = getListExpressionHelper(observable);
if (helper == null) {
return Collections.emptyList();
}
}
final Class helperClass = helper.getClass();

@@ -234,6 +238,23 @@ public static <E> List<ListChangeListener<? super E>> getListChangeListeners(Obs
}
} catch (ClassNotFoundException ex) { }

try {
final Class clazz = Class.forName(LIST_LISTENER_HELPER_GENERIC);
if (clazz.isAssignableFrom(helperClass)) {
try {
final Field field = clazz.getDeclaredField("changeListeners");
field.setAccessible(true);
final ListChangeListener<? super E>[] listeners = (ListChangeListener[])field.get(helper);
if (listeners != null) {
final Field sizeField = clazz.getDeclaredField("changeSize");
sizeField.setAccessible(true);
final int size = sizeField.getInt(helper);
return Arrays.asList(Arrays.copyOf(listeners, size));
}
} catch (Exception ex) { }
}
} catch (ClassNotFoundException ex) { }

return Collections.emptyList();
}

@@ -328,6 +349,19 @@ private static Object getExpressionHelper(Object bean) {
return null;
}

private static Object getListExpressionHelper(Object bean) {
Class clazz = bean.getClass();
while (clazz != Object.class) {
try {
final Field field = clazz.getDeclaredField("listenerHelper");
field.setAccessible(true);
return field.get(bean);
} catch (Exception ex) {}
clazz = clazz.getSuperclass();
}
return null;
}

private static List<InvalidationListener> getInvalidationListenerFromSingleInvalidationClass(Class clazz, Object helper) {
try {
final Field field = clazz.getDeclaredField("listener");
Original file line number Diff line number Diff line change
@@ -132,23 +132,49 @@ private static ChangeListener<Scene> getSceneChangeListener(Object anchor, Obser
return sceneChangeListener;
}

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

private static void doAcceleratorInstall(final ObservableList<MenuItem> items, final Scene scene) {
// we're given an observable list of menu items, which we will add an observer to
// so that when menu items are added or removed we can properly handle
// the addition or removal of accelerators into the scene.
items.addListener((ListChangeListener<MenuItem>) c -> {
while (c.next()) {
if (c.wasRemoved()) {
// remove accelerators from the scene
removeAcceleratorsFromScene(c.getRemoved(), scene);
}
// We need to store the ListChangeListener for later so that we can clean up, so ideally we'd use the items
// list as a key in a WeakHashMap with the ListChangeListener as the value. Unfortunately items can't be used
// as a key as is because its equals method compares the list contents which breaks immediately for empty
// lists.
// Instead we have to use an identity wrapper over the items as a key, but since the HashMap is weak, we also
// have to retain a reference to the identity wrapper class. This could be avoided if there was a
// WeakIdentityHashMap class available.
// One way is to use the ListChangeListener itself as the identity wrapper: IdentityWrapperListChangeListener
// since the items list will have a reference to it. We can create temporary instances of the
// IdentityWrapperListChangeListener when we need to retrieve items from the WeakHashMap to get the real one.
ListChangeListener<MenuItem> listChangeListener = new IdentityWrapperListChangeListener(items) {
@Override
public void onChanged(Change<? extends MenuItem> c) {
while (c.next()) {
if (c.wasRemoved()) {
removeAcceleratorsFromScene(c.getRemoved(), scene);
}

if (c.wasAdded()) {
ControlAcceleratorSupport.doAcceleratorInstall(c.getAddedSubList(), scene);
if (c.wasAdded()) {
doAcceleratorInstall(c.getAddedSubList(), scene);
}
}
}
});
};

// There should only ever be one ListChangeListener from ControlAcceleratorSupport on each items list
// If there is one somehow, this removes it from the list, and replaces the entry in the HashMap.
WeakReference<ListChangeListener<MenuItem>> previousW = menuListChangeListenerMap.get(listChangeListener);
ListChangeListener<MenuItem> previous = previousW == null ? null : previousW.get();
if (previous != null) {
items.removeListener(previous);
}

menuListChangeListenerMap.put(listChangeListener, new WeakReference<>(listChangeListener));
items.addListener(listChangeListener);
doAcceleratorInstall((List<MenuItem>)items, scene);
}

@@ -225,15 +251,15 @@ private static ChangeListener<KeyCombination> getListener(final Scene scene, Men

// --- Remove

public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, Tab anchor) {
public static void removeAcceleratorsFromScene(ObservableList<? extends MenuItem> items, Tab anchor) {
TabPane tabPane = anchor.getTabPane();
if (tabPane == null) return;

Scene scene = tabPane.getScene();
removeAcceleratorsFromScene(items, scene);
}

public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, TableColumnBase<?,?> anchor) {
public static void removeAcceleratorsFromScene(ObservableList<? extends MenuItem> items, TableColumnBase<?,?> anchor) {
ReadOnlyObjectProperty<? extends Control> controlProperty = getControlProperty(anchor);
if (controlProperty == null) return;

@@ -244,7 +270,7 @@ public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, T
removeAcceleratorsFromScene(items, scene);
}

public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, Node anchor) {
public static void removeAcceleratorsFromScene(ObservableList<? extends MenuItem> items, Node anchor) {
Scene scene = anchor.getScene();
if (scene == null) {
// The Node is not part of a Scene: Remove the Scene listener that was added
@@ -261,7 +287,20 @@ public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, N
removeAcceleratorsFromScene(items, scene);
}

public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, Scene scene) {
public static void removeAcceleratorsFromScene(ObservableList<? extends MenuItem> items, Scene scene) {
// use a temporary IdentityWrapperListChangeListener to get the actual one that's listening to items
WeakReference<ListChangeListener<MenuItem>> listenerW =
menuListChangeListenerMap.get(new IdentityWrapperListChangeListener(items));
if (listenerW != null) {
ListChangeListener<MenuItem> listChangeListener = listenerW.get();
if (listChangeListener != null) {
items.removeListener(listChangeListener);
}
}
removeAcceleratorsFromScene((List<MenuItem>)items, scene);
}

private static void removeAcceleratorsFromScene(List<? extends MenuItem> items, Scene scene) {
if (scene == null) {
return;
}
@@ -311,4 +350,50 @@ private static ReadOnlyObjectProperty<? extends Control> getControlProperty(Obje

return null;
}

/**
* We need to store all the listeners added to each ObservableList so that we can remove them. For this we need
* a map mapping ObservableList to various listeners such as ListChangeListeners, but the map needs to be a
* WeakHashMap.
* The ideal key to the WeakHashMap would be the ObservableList itself, except for the fact that its equals method
* compares the list contents. If a WeakIdentityHashMap existed, we could use that instead with the ObservableList
* as the key.
* We can't use an IdentityWrapper as the key to the HashMap because we need a strong reference (ideally from the
* ObservableList itself) to the IdentityWrapper else it'll be garbage collected.
* Since every ObservableList gets a ListChangeListener, we can use that as an IdentityWrapper and rely on the fact
* ObservableList has a strong reference to the ListChangeListener.
*/
static class IdentityWrapperListChangeListener implements ListChangeListener<MenuItem> {

ObservableList<? extends MenuItem> innerList;

public IdentityWrapperListChangeListener(ObservableList<? extends MenuItem> list) {
this.innerList = list;
}

@Override
public void onChanged(Change<? extends MenuItem> c) {
// In some cases this class is used as a key to fetch an item in a HashMap only, and then discarded
// In other cases the onChanged method is overridden and this is used as a ListChangeListener
}

@Override
public int hashCode() {
return System.identityHashCode(innerList);
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof IdentityWrapperListChangeListener)) {
return false;
}
IdentityWrapperListChangeListener that = (IdentityWrapperListChangeListener) o;
return innerList == that.innerList;
}
};
}


2 changes: 2 additions & 0 deletions modules/javafx.controls/src/test/addExports
Original file line number Diff line number Diff line change
@@ -7,6 +7,8 @@
--add-opens javafx.base/javafx.beans.property=ALL-UNNAMED
--add-opens javafx.base/com.sun.javafx.binding=ALL-UNNAMED
--add-exports javafx.base/com.sun.javafx.logging=ALL-UNNAMED
--add-opens javafx.base/com.sun.javafx.collections=ALL-UNNAMED
--add-opens javafx.base/javafx.collections=ALL-UNNAMED
#
--add-exports javafx.graphics/com.sun.javafx.application=ALL-UNNAMED
--add-exports javafx.graphics/com.sun.javafx.font=ALL-UNNAMED
Loading

5 comments on commit 8fd2943

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

@kevinrushforth
Copy link
Member

Choose a reason for hiding this comment

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

/tag 21+24

@openjdk
Copy link

@openjdk openjdk bot commented on 8fd2943 Jun 29, 2023

Choose a reason for hiding this comment

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

@kevinrushforth The tag 21+24 was successfully created.

@arapte
Copy link
Member

@arapte arapte commented on 8fd2943 Jul 6, 2023

Choose a reason for hiding this comment

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

/tag 21+25

@openjdk
Copy link

@openjdk openjdk bot commented on 8fd2943 Jul 6, 2023

Choose a reason for hiding this comment

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

@arapte The tag 21+25 was successfully created.

Please sign in to comment.