Skip to content

Commit a02e09d

Browse files
Jeanette Winzenburgarapte
authored andcommitted
8246195: ListViewSkin/Behavior: misbehavior on switching skin
Reviewed-by: arapte
1 parent 9749982 commit a02e09d

File tree

7 files changed

+260
-17
lines changed

7 files changed

+260
-17
lines changed

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -213,16 +213,20 @@ public ListViewBehavior(ListView<T> control) {
213213
ListView<T> control = getNode();
214214

215215
ListCellBehavior.removeAnchor(control);
216+
control.selectionModelProperty().removeListener(weakSelectionModelListener);
217+
if (control.getSelectionModel() != null) {
218+
control.getSelectionModel().getSelectedIndices().removeListener(weakSelectedIndicesListener);
219+
}
220+
control.itemsProperty().removeListener(weakItemsListener);
221+
if (control.getItems() != null) {
222+
control.getItems().removeListener(weakItemsListListener);
223+
}
224+
216225
if (tlFocus != null) tlFocus.dispose();
226+
control.removeEventFilter(KeyEvent.ANY, keyEventListener);
217227
super.dispose();
218-
219-
control.removeEventHandler(KeyEvent.ANY, keyEventListener);
220228
}
221229

222-
223-
224-
225-
226230
/**************************************************************************
227231
* State and Functions *
228232
*************************************************************************/

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListViewSkin.java

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2010, 2018, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -37,6 +37,7 @@
3737
import javafx.collections.ObservableList;
3838
import javafx.collections.ObservableMap;
3939
import javafx.collections.WeakListChangeListener;
40+
import javafx.collections.WeakMapChangeListener;
4041
import javafx.event.EventHandler;
4142
import javafx.geometry.Orientation;
4243
import javafx.scene.AccessibleAction;
@@ -104,7 +105,6 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
104105
private Node placeholderNode;
105106

106107
private ObservableList<T> listViewItems;
107-
private final InvalidationListener itemsChangeListener = observable -> updateListViewItems();
108108

109109
private boolean needCellsRebuilt = true;
110110
private boolean needCellsReconfigured = false;
@@ -129,6 +129,9 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
129129
}
130130
};
131131

132+
private WeakMapChangeListener<Object, Object> weakPropertiesMapListener =
133+
new WeakMapChangeListener<>(propertiesMapListener);
134+
132135
private final ListChangeListener<T> listViewItemsListener = new ListChangeListener<T>() {
133136
@Override public void onChanged(Change<? extends T> c) {
134137
while (c.next()) {
@@ -166,6 +169,12 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
166169
new WeakListChangeListener<T>(listViewItemsListener);
167170

168171

172+
private final InvalidationListener itemsChangeListener = observable -> updateListViewItems();
173+
174+
private WeakInvalidationListener
175+
weakItemsChangeListener = new WeakInvalidationListener(itemsChangeListener);
176+
177+
private EventHandler<MouseEvent> ml;
169178

170179
/***************************************************************************
171180
* *
@@ -208,7 +217,7 @@ public ListViewSkin(final ListView<T> control) {
208217
flow.setFixedCellSize(control.getFixedCellSize());
209218
getChildren().add(flow);
210219

211-
EventHandler<MouseEvent> ml = event -> {
220+
ml = event -> {
212221
// RT-15127: cancel editing on scroll. This is a bit extreme
213222
// (we are cancelling editing on touching the scrollbars).
214223
// This can be improved at a later date.
@@ -230,11 +239,11 @@ public ListViewSkin(final ListView<T> control) {
230239

231240
updateItemCount();
232241

233-
control.itemsProperty().addListener(new WeakInvalidationListener(itemsChangeListener));
242+
control.itemsProperty().addListener(weakItemsChangeListener);
234243

235244
final ObservableMap<Object, Object> properties = control.getProperties();
236245
properties.remove(Properties.RECREATE);
237-
properties.addListener(propertiesMapListener);
246+
properties.addListener(weakPropertiesMapListener);
238247

239248
// Register listeners
240249
registerChangeListener(control.itemsProperty(), o -> updateListViewItems());
@@ -263,6 +272,20 @@ public ListViewSkin(final ListView<T> control) {
263272

264273
/** {@inheritDoc} */
265274
@Override public void dispose() {
275+
if (getSkinnable() == null) return;
276+
// listener cleanup fixes side-effects (NPE on refresh, setItems, modifyItems)
277+
getSkinnable().getProperties().removeListener(weakPropertiesMapListener);
278+
getSkinnable().itemsProperty().removeListener(weakItemsChangeListener);
279+
if (listViewItems != null) {
280+
listViewItems.removeListener(weakListViewItemsListener);
281+
listViewItems = null;
282+
}
283+
// flow related cleanup
284+
// leaking without nulling factory
285+
flow.setCellFactory(null);
286+
// for completeness - but no effect with/out?
287+
flow.getVbar().removeEventFilter(MouseEvent.MOUSE_PRESSED, ml);
288+
flow.getHbar().removeEventFilter(MouseEvent.MOUSE_PRESSED, ml);
266289
super.dispose();
267290

268291
if (behavior != null) {

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualContainerBase.java

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2010, 2017, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -25,6 +25,7 @@
2525

2626
package javafx.scene.control.skin;
2727

28+
import javafx.event.EventHandler;
2829
import javafx.scene.control.Control;
2930
import javafx.scene.control.IndexedCell;
3031
import javafx.scene.control.ScrollToEvent;
@@ -53,6 +54,7 @@ public abstract class VirtualContainerBase<C extends Control, I extends IndexedC
5354
*/
5455
private final VirtualFlow<I> flow;
5556

57+
private EventHandler<? super ScrollToEvent<Integer>> scrollToEventHandler;
5658

5759

5860
/***************************************************************************
@@ -69,7 +71,7 @@ public VirtualContainerBase(final C control) {
6971
super(control);
7072
flow = createVirtualFlow();
7173

72-
control.addEventHandler(ScrollToEvent.scrollToTopIndex(), event -> {
74+
scrollToEventHandler = event -> {
7375
// Fix for RT-24630: The row count in VirtualFlow was incorrect
7476
// (normally zero), so the scrollTo call was misbehaving.
7577
if (itemCountDirty) {
@@ -78,7 +80,8 @@ public VirtualContainerBase(final C control) {
7880
itemCountDirty = false;
7981
}
8082
flow.scrollToTop(event.getScrollTarget());
81-
});
83+
};
84+
control.addEventHandler(ScrollToEvent.scrollToTopIndex(), scrollToEventHandler);
8285
}
8386

8487

@@ -123,6 +126,17 @@ protected VirtualFlow<I> createVirtualFlow() {
123126
return new VirtualFlow<>();
124127
}
125128

129+
/**
130+
* {@inheritDoc} <p>
131+
* Overridden to remove EventHandler.
132+
*/
133+
@Override
134+
public void dispose() {
135+
if (getSkinnable() == null) return;
136+
getSkinnable().removeEventHandler(ScrollToEvent.scrollToTopIndex(), scrollToEventHandler);
137+
super.dispose();
138+
}
139+
126140
/**
127141
* Get the virtualized container.
128142
* Subclasses can invoke this method to get the VirtualFlow instance.
Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
/*
2+
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation. Oracle designates this
8+
* particular file as subject to the "Classpath" exception as provided
9+
* by Oracle in the LICENSE file that accompanied this code.
10+
*
11+
* This code is distributed in the hope that it will be useful, but WITHOUT
12+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
13+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
14+
* version 2 for more details (a copy is included in the LICENSE file that
15+
* accompanied this code).
16+
*
17+
* You should have received a copy of the GNU General Public License version
18+
* 2 along with this work; if not, write to the Free Software Foundation,
19+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
20+
*
21+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
22+
* or visit www.oracle.com if you need additional information or have any
23+
* questions.
24+
*/
25+
26+
package test.com.sun.javafx.scene.control.behavior;
27+
28+
import java.lang.ref.WeakReference;
29+
30+
import org.junit.After;
31+
import org.junit.Before;
32+
import org.junit.Test;
33+
34+
import com.sun.javafx.scene.control.behavior.BehaviorBase;
35+
import com.sun.javafx.scene.control.behavior.ListCellBehavior;
36+
37+
import static javafx.collections.FXCollections.*;
38+
import static org.junit.Assert.*;
39+
import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;
40+
41+
import javafx.scene.control.ListView;
42+
43+
/**
44+
* Test for misbehavior of individual implementations that turned
45+
* up in binch testing.
46+
*
47+
*/
48+
public class BehaviorCleanupTest {
49+
50+
// ---------- ListView
51+
52+
/**
53+
* Test cleanup of listener to itemsProperty.
54+
*/
55+
@Test
56+
public void testListViewBehaviorDisposeSetItems() {
57+
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
58+
WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
59+
weakRef.get().dispose();
60+
int last = 1;
61+
ListCellBehavior.setAnchor(listView, last, false);
62+
listView.setItems(observableArrayList("other", "again"));
63+
assertEquals("sanity: anchor unchanged", last, listView.getProperties().get("anchor"));
64+
listView.getItems().remove(0);
65+
assertEquals("anchor must not be updated on items modification when disposed",
66+
last, listView.getProperties().get("anchor"));
67+
}
68+
69+
@Test
70+
public void testListViewBehaviorSetItems() {
71+
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
72+
createBehavior(listView);
73+
int last = 1;
74+
ListCellBehavior.setAnchor(listView, last, false);
75+
listView.setItems(observableArrayList("other", "again"));
76+
assertEquals("sanity: anchor unchanged", last, listView.getProperties().get("anchor"));
77+
listView.getItems().remove(0);
78+
assertEquals("anchor must be updated on items modification",
79+
last -1, listView.getProperties().get("anchor"));
80+
}
81+
82+
/**
83+
* Test cleanup of itemsList listener in ListViewBehavior.
84+
*/
85+
@Test
86+
public void testListViewBehaviorDisposeRemoveItem() {
87+
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
88+
WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
89+
weakRef.get().dispose();
90+
int last = 1;
91+
ListCellBehavior.setAnchor(listView, last, false);
92+
listView.getItems().remove(0);
93+
assertEquals("anchor must not be updated on items modification when disposed",
94+
last,
95+
listView.getProperties().get("anchor"));
96+
}
97+
98+
@Test
99+
public void testListViewBehaviorRemoveItem() {
100+
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
101+
createBehavior(listView);
102+
int last = 1;
103+
ListCellBehavior.setAnchor(listView, last, false);
104+
assertEquals("behavior must set anchor on select", last, listView.getProperties().get("anchor"));
105+
listView.getItems().remove(0);
106+
assertEquals("anchor must be updated on items modification",
107+
last -1, listView.getProperties().get("anchor"));
108+
}
109+
110+
/**
111+
* Test cleanup of selection listeners in ListViewBehavior.
112+
*/
113+
@Test
114+
public void testListViewBehaviorDisposeSelect() {
115+
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
116+
WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
117+
listView.getSelectionModel().select(1);
118+
weakRef.get().dispose();
119+
listView.getSelectionModel().select(0);
120+
assertNull("anchor must remain cleared on selecting when disposed",
121+
listView.getProperties().get("anchor"));
122+
}
123+
124+
@Test
125+
public void testListViewBehaviorSelect() {
126+
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
127+
createBehavior(listView);
128+
int last = 1;
129+
listView.getSelectionModel().select(last);
130+
assertEquals("anchor must be set", last, listView.getProperties().get("anchor"));
131+
}
132+
133+
@Test
134+
public void testListViewBehaviorDispose() {
135+
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
136+
WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
137+
listView.getSelectionModel().select(1);
138+
weakRef.get().dispose();
139+
assertNull("anchor must be cleared after dispose", listView.getProperties().get("anchor"));
140+
}
141+
142+
//------------------ setup/cleanup
143+
144+
@After
145+
public void cleanup() {
146+
Thread.currentThread().setUncaughtExceptionHandler(null);
147+
}
148+
149+
@Before
150+
public void setup() {
151+
Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
152+
if (throwable instanceof RuntimeException) {
153+
throw (RuntimeException)throwable;
154+
} else {
155+
Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable);
156+
}
157+
});
158+
}
159+
160+
}

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorMemoryLeakTest.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@ public static Collection<Object[]> data() {
8484
// step 1: file issues (where not yet done), add informal ignore to entry
8585
// step 2: fix and remove from list
8686
List<Class<? extends Control>> leakingClasses = List.of(
87-
ListView.class,
8887
PasswordField.class,
8988
TableView.class,
9089
TextArea.class,

0 commit comments

Comments
 (0)