Skip to content

Commit

Permalink
8241455: Memory leak on replacing selection/focusModel
Browse files Browse the repository at this point in the history 
Reviewed-by: arapte, aghaisas
  • Loading branch information
@arapte
Jeanette Winzenburg authored and arapte committed Apr 6, 2020
1 parent 5906521 commit 560ef17
Show file tree
Hide file tree
Showing 5 changed files with 337 additions and 18 deletions.
22 changes: 15 additions & 7 deletions modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2020, 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
Expand Down Expand Up @@ -29,9 +29,11 @@
import javafx.beans.property.ObjectPropertyBase;
import javafx.beans.property.SimpleObjectProperty;
import javafx.beans.value.ChangeListener;
import javafx.beans.value.WeakChangeListener;
import javafx.collections.FXCollections;
import javafx.collections.ListChangeListener;
import javafx.collections.ObservableList;
import javafx.collections.WeakListChangeListener;
import javafx.beans.property.ReadOnlyBooleanProperty;
import javafx.beans.property.ReadOnlyBooleanWrapper;
import javafx.event.ActionEvent;
Expand Down Expand Up @@ -503,6 +505,9 @@ public void hide() {
// package for testing
static class ChoiceBoxSelectionModel<T> extends SingleSelectionModel<T> {
private final ChoiceBox<T> choiceBox;
private ChangeListener<ObservableList<T>> itemsObserver;
private ListChangeListener<T> itemsContentObserver;
private WeakListChangeListener<T> weakItemsContentObserver;

public ChoiceBoxSelectionModel(final ChoiceBox<T> cb) {
if (cb == null) {
Expand All @@ -520,7 +525,7 @@ public ChoiceBoxSelectionModel(final ChoiceBox<T> cb) {
*/

// watching for changes to the items list content
final ListChangeListener<T> itemsContentObserver = c -> {
itemsContentObserver = c -> {
if (choiceBox.getItems() == null || choiceBox.getItems().isEmpty()) {
setSelectedIndex(-1);
} else if (getSelectedIndex() == -1 && getSelectedItem() != null) {
Expand All @@ -530,17 +535,18 @@ public ChoiceBoxSelectionModel(final ChoiceBox<T> cb) {
}
}
};
weakItemsContentObserver = new WeakListChangeListener<>(itemsContentObserver);
if (this.choiceBox.getItems() != null) {
this.choiceBox.getItems().addListener(itemsContentObserver);
this.choiceBox.getItems().addListener(weakItemsContentObserver);
}

// watching for changes to the items list
ChangeListener<ObservableList<T>> itemsObserver = (valueModel, oldList, newList) -> {
itemsObserver = (valueModel, oldList, newList) -> {
if (oldList != null) {
oldList.removeListener(itemsContentObserver);
oldList.removeListener(weakItemsContentObserver);
}
if (newList != null) {
newList.addListener(itemsContentObserver);
newList.addListener(weakItemsContentObserver);
}
setSelectedIndex(-1);
if (getSelectedItem() != null) {
Expand All @@ -550,7 +556,9 @@ public ChoiceBoxSelectionModel(final ChoiceBox<T> cb) {
}
}
};
this.choiceBox.itemsProperty().addListener(itemsObserver);
// TBD: use pattern as f.i. in listView selectionModel (invalidationListener to really
// get all changes - including list of same content - of the list-valued property)
this.choiceBox.itemsProperty().addListener(new WeakChangeListener<>(itemsObserver));
}

// API Implementation
Expand Down
9 changes: 6 additions & 3 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2020, 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
Expand Down Expand Up @@ -41,6 +41,7 @@
import javafx.beans.value.WritableValue;
import javafx.collections.ListChangeListener;
import javafx.collections.ObservableList;
import javafx.collections.WeakListChangeListener;
import javafx.geometry.Side;
import javafx.scene.AccessibleAttribute;
import javafx.scene.AccessibleRole;
Expand Down Expand Up @@ -672,14 +673,16 @@ public StyleableProperty<Number> getStyleableProperty(TabPane n) {
static class TabPaneSelectionModel extends SingleSelectionModel<Tab> {
private final TabPane tabPane;

private ListChangeListener<Tab> itemsContentObserver;

public TabPaneSelectionModel(final TabPane t) {
if (t == null) {
throw new NullPointerException("TabPane can not be null");
}
this.tabPane = t;

// watching for changes to the items list content
final ListChangeListener<Tab> itemsContentObserver = c -> {
itemsContentObserver = c -> {
while (c.next()) {
for (Tab tab : c.getRemoved()) {
if (tab != null && !tabPane.getTabs().contains(tab)) {
Expand Down Expand Up @@ -710,7 +713,7 @@ public TabPaneSelectionModel(final TabPane t) {
}
};
if (this.tabPane.getTabs() != null) {
this.tabPane.getTabs().addListener(itemsContentObserver);
this.tabPane.getTabs().addListener(new WeakListChangeListener<>(itemsContentObserver));
}
}

Expand Down
9 changes: 6 additions & 3 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2020, 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
Expand Down Expand Up @@ -3406,12 +3406,13 @@ public TreeTableViewFocusModel(final TreeTableView<S> treeTableView) {
TreeTablePosition<S,?> pos = new TreeTablePosition<>(treeTableView, focusRow, null);
setFocusedCell(pos);

treeTableView.showRootProperty().addListener(o -> {
showRootListener = obs -> {
if (isFocused(0)) {
focus(-1);
focus(0);
}
});
};
treeTableView.showRootProperty().addListener(new WeakInvalidationListener(showRootListener));

focusedCellProperty().addListener(o -> {
treeTableView.notifyAccessibleAttributeChanged(AccessibleAttribute.FOCUS_ITEM);
Expand All @@ -3425,6 +3426,8 @@ public TreeTableViewFocusModel(final TreeTableView<S> treeTableView) {
private final WeakChangeListener<TreeItem<S>> weakRootPropertyListener =
new WeakChangeListener<>(rootPropertyListener);

private final InvalidationListener showRootListener;

private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
if (oldRoot != null && weakTreeItemListener != null) {
oldRoot.removeEventHandler(TreeItem.<S>expandedItemCountChangeEvent(), weakTreeItemListener);
Expand Down
18 changes: 13 additions & 5 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2008, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2008, 2020, 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
Expand Down Expand Up @@ -32,6 +32,8 @@

import javafx.application.Platform;
import javafx.beans.DefaultProperty;
import javafx.beans.InvalidationListener;
import javafx.beans.WeakInvalidationListener;
import javafx.beans.property.BooleanProperty;
import javafx.beans.property.DoubleProperty;
import javafx.beans.property.ObjectProperty;
Expand Down Expand Up @@ -1295,9 +1297,10 @@ public TreeViewBitSetSelectionModel(final TreeView<T> treeView) {

this.treeView = treeView;
this.treeView.rootProperty().addListener(weakRootPropertyListener);
this.treeView.showRootProperty().addListener(o -> {
showRootListener = o -> {
shiftSelection(0, treeView.isShowRoot() ? 1 : -1, null);
});
};
this.treeView.showRootProperty().addListener(new WeakInvalidationListener(showRootListener));

updateTreeEventListener(null, treeView.getRoot());

Expand All @@ -1310,6 +1313,7 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
}

if (newRoot != null) {
//PENDING why create a new weak eventHandler?
weakTreeItemListener = new WeakEventHandler<>(treeItemListener);
newRoot.addEventHandler(TreeItem.<T>expandedItemCountChangeEvent(), weakTreeItemListener);
}
Expand Down Expand Up @@ -1456,6 +1460,7 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {

private WeakEventHandler<TreeModificationEvent<T>> weakTreeItemListener;

private InvalidationListener showRootListener;


/***********************************************************************
Expand Down Expand Up @@ -1595,12 +1600,13 @@ public TreeViewFocusModel(final TreeView<T> treeView) {
focus(0);
}

treeView.showRootProperty().addListener(o -> {
showRootListener = obs -> {
if (isFocused(0)) {
focus(-1);
focus(0);
}
});
};
treeView.showRootProperty().addListener(new WeakInvalidationListener(showRootListener));

focusedIndexProperty().addListener(o -> {
treeView.notifyAccessibleAttributeChanged(AccessibleAttribute.FOCUS_ITEM);
Expand All @@ -1614,6 +1620,8 @@ public TreeViewFocusModel(final TreeView<T> treeView) {
private final WeakChangeListener<TreeItem<T>> weakRootPropertyListener =
new WeakChangeListener<>(rootPropertyListener);

private final InvalidationListener showRootListener;

private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
if (oldRoot != null && weakTreeItemListener != null) {
oldRoot.removeEventHandler(TreeItem.<T>expandedItemCountChangeEvent(), weakTreeItemListener);
Expand Down

0 comments on commit 560ef17

Please sign in to comment.