8237926: Potential memory leak of model data in javafx.scene.control.…

…ListView Reviewed-by: kcr, aghaisas
openjdk · Mar 5, 2020 · 337ed72 · 337ed72
1 parent 960f039
commit 337ed72
Show file tree

Hide file tree

Showing 4 changed files with 12 additions and 39 deletions.
diff --git a/...trols/src/main/java/com/sun/javafx/scene/control/SelectedItemsReadOnlyObservableList.java b/...trols/src/main/java/com/sun/javafx/scene/control/SelectedItemsReadOnlyObservableList.java
@@ -27,7 +27,6 @@
 import javafx.collections.ListChangeListener;
 import javafx.collections.ObservableList;
 import javafx.collections.ObservableListBase;
-import javafx.collections.WeakListChangeListener;
 
 import java.lang.ref.WeakReference;
 import java.util.ArrayList;
@@ -38,20 +37,7 @@ public abstract class SelectedItemsReadOnlyObservableList<E> extends ObservableL
 
     // This is the actual observable list of selected indices used in the selection model
     private final ObservableList<Integer> selectedIndices;
-
-    private ObservableList<E> itemsList;
-
-    private boolean itemsListChanged = false;
-    private ListChangeListener.Change<? extends E> itemsListChange;
-    private final ListChangeListener itemsListListener = c -> {
-        itemsListChanged = true;
-        itemsListChange = c;
-    };
-    private final WeakListChangeListener weakItemsListListener =
-            new WeakListChangeListener(itemsListListener);
-
     private final Supplier<Integer> modelSizeSupplier;
-
     private final List<WeakReference<E>> itemsRefList;
 
     public SelectedItemsReadOnlyObservableList(ObservableList<Integer> selectedIndices, Supplier<Integer> modelSizeSupplier) {
@@ -100,9 +86,6 @@ public SelectedItemsReadOnlyObservableList(ObservableList<Integer> selectedIndic
                 itemsRefList.add(new WeakReference<>(getModelItem(selectedIndex)));
             }
 
-            itemsListChanged = false;
-            itemsListChange = null;
-
             endChange();
         });
     }
@@ -120,17 +103,6 @@ public int size() {
         return selectedIndices.size();
     }
 
-    // Used by ListView and TableView to allow for improved handling.
-    public void setItemsList(ObservableList<E> itemsList) {
-        if (this.itemsList != null) {
-            this.itemsList.removeListener(weakItemsListListener);
-        }
-        this.itemsList = itemsList;
-        if (itemsList != null) {
-            itemsList.addListener(weakItemsListListener);
-        }
-    }
-
     private E _getModelItem(int index) {
         if (index >= modelSizeSupplier.get()) {
             // attempt to return from the itemsRefList instead

diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java b/modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java
@@ -31,7 +31,6 @@
 import java.util.List;
 
 import com.sun.javafx.scene.control.Properties;
-import com.sun.javafx.scene.control.SelectedItemsReadOnlyObservableList;
 import com.sun.javafx.scene.control.behavior.ListCellBehavior;
 import javafx.beans.InvalidationListener;
 import javafx.beans.Observable;
@@ -1221,9 +1220,6 @@ public ListViewBitSetSelectionModel(final ListView<T> listView) {
 
             this.listView = listView;
 
-            ((SelectedItemsReadOnlyObservableList)getSelectedItems()).setItemsList(listView.getItems());
-
-
             /*
              * The following two listeners are used in conjunction with
              * SelectionModel.select(T obj) to allow for a developer to select
@@ -1238,7 +1234,6 @@ public ListViewBitSetSelectionModel(final ListView<T> listView) {
                 @Override public void invalidated(Observable observable) {
                     ObservableList<T> oldItems = weakItemsRef.get();
                     weakItemsRef = new WeakReference<>(listView.getItems());
-                    ((SelectedItemsReadOnlyObservableList)getSelectedItems()).setItemsList(listView.getItems());
                     updateItemsObserver(oldItems, listView.getItems());
                 }
             };

diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java b/modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java
@@ -40,7 +40,6 @@
 import com.sun.javafx.scene.control.Logging;
 import com.sun.javafx.scene.control.Properties;
 import com.sun.javafx.scene.control.SelectedCellsMap;
-import com.sun.javafx.scene.control.SelectedItemsReadOnlyObservableList;
 import com.sun.javafx.scene.control.behavior.TableCellBehavior;
 import com.sun.javafx.scene.control.behavior.TableCellBehaviorBase;
 
@@ -2104,8 +2103,6 @@ public TableViewArrayListSelectionModel(final TableView<S> tableView) {
                     ObservableList<S> oldItems = weakItemsRef.get();
                     weakItemsRef = new WeakReference<>(tableView.getItems());
                     updateItemsObserver(oldItems, tableView.getItems());
-
-                    ((SelectedItemsReadOnlyObservableList)getSelectedItems()).setItemsList(tableView.getItems());
                 }
             };
             this.tableView.itemsProperty().addListener(itemsPropertyListener);
@@ -2142,7 +2139,6 @@ public TableViewArrayListSelectionModel(final TableView<S> tableView) {
             // watching for changes to the items list content
             ObservableList<S> items = getTableView().getItems();
             if (items != null) {
-                ((SelectedItemsReadOnlyObservableList)getSelectedItems()).setItemsList(items);
                 items.addListener(weakItemsContentListener);
             }
 

diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
@@ -1982,10 +1982,20 @@ public void testListViewLeak() {
         ObservableList<String> items = FXCollections.observableArrayList();
         WeakReference<ListView<String>> listViewRef = new WeakReference<>(new ListView<>(items));
         attemptGC(listViewRef, 10);
-        assertNull("ListView has a leak.", listViewRef.get());
+        assertNull("ListView is not GCed.", listViewRef.get());
     }
 
-    private void attemptGC(WeakReference<ListView<String>> weakRef, int n) {
+    @Test
+    public void testItemLeak() {
+        WeakReference<String> itemRef = new WeakReference<>(new String("Leak Item"));
+        ObservableList<String> items = FXCollections.observableArrayList(itemRef.get());
+        ListView<String> listView = new ListView<>(items);
+        items.clear();
+        attemptGC(itemRef, 10);
+        assertNull("ListView item is not GCed.", itemRef.get());
+    }
+
+    private void attemptGC(WeakReference<? extends Object> weakRef, int n) {
         for (int i = 0; i < n; i++) {
             System.gc();
             System.runFinalization();