Skip to content

Commit 6abbe08

Browse files
author
Andy Goryachev
committed
8295809: TreeTableViewSkin: memory leak when changing skin
Reviewed-by: aghaisas
1 parent 07e693b commit 6abbe08

File tree

4 files changed

+135
-158
lines changed

4 files changed

+135
-158
lines changed

‎modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java

+36-38
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2010, 2022, 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,8 +25,21 @@
2525

2626
package javafx.scene.control.skin;
2727

28-
import com.sun.javafx.scene.control.behavior.BehaviorBase;
28+
import java.util.ArrayList;
29+
import java.util.Collections;
30+
import java.util.List;
31+
32+
import javafx.beans.property.DoubleProperty;
33+
import javafx.beans.property.ObjectProperty;
2934
import javafx.collections.FXCollections;
35+
import javafx.collections.ObservableList;
36+
import javafx.css.CssMetaData;
37+
import javafx.css.Styleable;
38+
import javafx.css.StyleableDoubleProperty;
39+
import javafx.css.StyleableProperty;
40+
import javafx.css.converter.SizeConverter;
41+
import javafx.scene.AccessibleAttribute;
42+
import javafx.scene.Node;
3043
import javafx.scene.control.Control;
3144
import javafx.scene.control.TableColumnBase;
3245
import javafx.scene.control.TreeItem;
@@ -36,24 +49,10 @@
3649
import javafx.scene.control.TreeTableRow;
3750
import javafx.scene.control.TreeTableView;
3851

39-
import java.util.ArrayList;
40-
import java.util.Collections;
41-
import java.util.List;
42-
43-
import javafx.beans.property.DoubleProperty;
44-
import javafx.scene.AccessibleAttribute;
45-
import javafx.scene.Node;
46-
import javafx.css.StyleableDoubleProperty;
47-
import javafx.css.CssMetaData;
48-
49-
import javafx.css.converter.SizeConverter;
52+
import com.sun.javafx.scene.control.ListenerHelper;
53+
import com.sun.javafx.scene.control.behavior.BehaviorBase;
5054
import com.sun.javafx.scene.control.behavior.TreeTableRowBehavior;
5155

52-
import javafx.beans.property.ObjectProperty;
53-
import javafx.collections.ObservableList;
54-
import javafx.css.Styleable;
55-
import javafx.css.StyleableProperty;
56-
5756
/**
5857
* Default skin implementation for the {@link TreeTableRow} control.
5958
*
@@ -74,9 +73,6 @@ public class TreeTableRowSkin<T> extends TableRowSkinBase<TreeItem<T>, TreeTable
7473
private boolean disclosureNodeDirty = true;
7574
private Node graphic;
7675
private final BehaviorBase<TreeTableRow<T>> behavior;
77-
78-
private TreeTableViewSkin treeTableViewSkin;
79-
8076
private boolean childrenDirty = false;
8177

8278

@@ -99,14 +95,16 @@ public TreeTableRowSkin(TreeTableRow<T> control) {
9995

10096
// install default input map for the TreeTableRow control
10197
behavior = new TreeTableRowBehavior<>(control);
102-
// control.setInputMap(behavior.getInputMap());
10398

10499
updateTreeItem();
105-
updateTableViewSkin();
106100

107-
registerChangeListener(control.treeTableViewProperty(), e -> updateTableViewSkin());
108-
registerChangeListener(control.indexProperty(), e -> updateCells = true);
109-
registerChangeListener(control.treeItemProperty(), e -> {
101+
ListenerHelper lh = ListenerHelper.get(this);
102+
103+
lh.addChangeListener(control.indexProperty(), (ev) -> {
104+
updateCells = true;
105+
});
106+
107+
lh.addChangeListener(control.treeItemProperty(), (ev) -> {
110108
updateTreeItem();
111109
// There used to be an isDirty = true statement here, but this was
112110
// determined to be unnecessary and led to performance issues such as
@@ -118,14 +116,15 @@ public TreeTableRowSkin(TreeTableRow<T> control) {
118116

119117
// FIXME: replace listener to fixedCellSize with direct lookup - JDK-8277000
120118
private void setupTreeTableViewListeners() {
119+
ListenerHelper lh = ListenerHelper.get(this);
121120
TreeTableView<T> treeTableView = getSkinnable().getTreeTableView();
122121
if (treeTableView == null) {
123-
registerInvalidationListener(getSkinnable().treeTableViewProperty(), e -> {
122+
lh.addInvalidationListener(getSkinnable().treeTableViewProperty(), (ev) -> {
124123
unregisterInvalidationListeners(getSkinnable().treeTableViewProperty());
125124
setupTreeTableViewListeners();
126125
});
127126
} else {
128-
registerChangeListener(treeTableView.treeColumnProperty(), e -> {
127+
lh.addChangeListener(treeTableView.treeColumnProperty(), (ev) -> {
129128
// Fix for RT-27782: Need to set isDirty to true, rather than the
130129
// cheaper updateCells, as otherwise the text indentation will not
131130
// be recalculated in TreeTableCellSkin.calculateIndentation()
@@ -135,7 +134,7 @@ private void setupTreeTableViewListeners() {
135134

136135
DoubleProperty fixedCellSizeProperty = getTreeTableView().fixedCellSizeProperty();
137136
if (fixedCellSizeProperty != null) {
138-
registerChangeListener(fixedCellSizeProperty, e -> {
137+
lh.addChangeListener(fixedCellSizeProperty, (ev) -> {
139138
fixedCellSize = fixedCellSizeProperty.get();
140139
fixedCellSizeEnabled = fixedCellSize > 0;
141140
});
@@ -146,7 +145,9 @@ private void setupTreeTableViewListeners() {
146145
// When in fixed cell size mode, we must listen to the width of the virtual flow, so
147146
// that when it changes, we can appropriately add / remove cells that may or may not
148147
// be required (because we remove all cells that are not visible).
149-
registerChangeListener(getVirtualFlow().widthProperty(), e -> treeTableView.requestLayout());
148+
lh.addChangeListener(getVirtualFlow().widthProperty(), (ev) -> {
149+
treeTableView.requestLayout();
150+
});
150151
}
151152
}
152153
}
@@ -382,16 +383,13 @@ private void updateDisclosureNodeAndGraphic() {
382383
}
383384
}
384385

385-
private void updateTableViewSkin() {
386-
TreeTableView<T> tableView = getSkinnable().getTreeTableView();
387-
if (tableView != null && tableView.getSkin() instanceof TreeTableViewSkin) {
388-
treeTableViewSkin = (TreeTableViewSkin)tableView.getSkin();
389-
}
390-
}
391-
392386
// test-only
393387
TreeTableViewSkin<T> getTableViewSkin() {
394-
return treeTableViewSkin;
388+
TreeTableView<T> t = getSkinnable().getTreeTableView();
389+
if (t != null && t.getSkin() instanceof TreeTableViewSkin) {
390+
return (TreeTableViewSkin)t.getSkin();
391+
}
392+
return null;
395393
}
396394

397395
// test-only

‎modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableViewSkin.java

+77-64
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2012, 2022, 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,12 +25,6 @@
2525

2626
package javafx.scene.control.skin;
2727

28-
import com.sun.javafx.scene.control.TreeTableViewBackingList;
29-
import javafx.event.WeakEventHandler;
30-
import javafx.scene.control.*;
31-
32-
import com.sun.javafx.scene.control.behavior.TreeTableViewBehavior;
33-
3428
import java.lang.ref.WeakReference;
3529
import java.util.ArrayList;
3630
import java.util.List;
@@ -43,10 +37,21 @@
4337
import javafx.scene.AccessibleAction;
4438
import javafx.scene.AccessibleAttribute;
4539
import javafx.scene.Node;
46-
import javafx.scene.control.TreeItem.TreeModificationEvent;
40+
import javafx.scene.control.Control;
41+
import javafx.scene.control.TreeItem;
42+
import javafx.scene.control.TreeTableCell;
43+
import javafx.scene.control.TreeTableColumn;
44+
import javafx.scene.control.TreeTablePosition;
45+
import javafx.scene.control.TreeTableRow;
46+
import javafx.scene.control.TreeTableView;
4747
import javafx.scene.input.MouseEvent;
4848
import javafx.scene.layout.StackPane;
4949

50+
import com.sun.javafx.scene.control.IDisconnectable;
51+
import com.sun.javafx.scene.control.ListenerHelper;
52+
import com.sun.javafx.scene.control.TreeTableViewBackingList;
53+
import com.sun.javafx.scene.control.behavior.TreeTableViewBehavior;
54+
5055
/**
5156
* Default skin implementation for the {@link TreeTableView} control.
5257
*
@@ -66,46 +71,7 @@ public class TreeTableViewSkin<T> extends TableViewSkinBase<T, TreeItem<T>, Tree
6671

6772
private WeakReference<TreeItem<T>> weakRootRef;
6873
private final TreeTableViewBehavior<T> behavior;
69-
70-
71-
72-
/* *************************************************************************
73-
* *
74-
* Listeners *
75-
* *
76-
**************************************************************************/
77-
78-
private EventHandler<TreeItem.TreeModificationEvent<T>> rootListener = e -> {
79-
if (e.wasAdded() && e.wasRemoved() && e.getAddedSize() == e.getRemovedSize()) {
80-
// Fix for RT-14842, where the children of a TreeItem were changing,
81-
// but because the overall item count was staying the same, there was
82-
// no event being fired to the skin to be informed that the items
83-
// had changed. So, here we just watch for the case where the number
84-
// of items being added is equal to the number of items being removed.
85-
markItemCountDirty();
86-
getSkinnable().requestLayout();
87-
} else if (e.getEventType().equals(TreeItem.valueChangedEvent())) {
88-
// Fix for RT-14971 and RT-15338.
89-
requestRebuildCells();
90-
} else {
91-
// Fix for RT-20090. We are checking to see if the event coming
92-
// from the TreeItem root is an event where the count has changed.
93-
EventType<?> eventType = e.getEventType();
94-
while (eventType != null) {
95-
if (eventType.equals(TreeItem.<T>expandedItemCountChangeEvent())) {
96-
markItemCountDirty();
97-
getSkinnable().requestLayout();
98-
break;
99-
}
100-
eventType = eventType.getSuperType();
101-
}
102-
}
103-
104-
// fix for RT-37853
105-
getSkinnable().edit(-1, null);
106-
};
107-
108-
private WeakEventHandler<TreeModificationEvent<T>> weakRootListener;
74+
private IDisconnectable rootListener;
10975

11076

11177

@@ -127,13 +93,14 @@ public TreeTableViewSkin(final TreeTableView<T> control) {
12793

12894
// install default input map for the TreeTableView control
12995
behavior = new TreeTableViewBehavior<>(control);
130-
// control.setInputMap(behavior.getInputMap());
13196

13297
flow.setFixedCellSize(control.getFixedCellSize());
13398
flow.setCellFactory(flow -> createCell());
13499

135100
setRoot(getSkinnable().getRoot());
136101

102+
ListenerHelper lh = ListenerHelper.get(this);
103+
137104
EventHandler<MouseEvent> ml = event -> {
138105
// This ensures that the table maintains the focus, even when the vbar
139106
// and hbar controls inside the flow are clicked. Without this, the
@@ -144,8 +111,8 @@ public TreeTableViewSkin(final TreeTableView<T> control) {
144111
control.requestFocus();
145112
}
146113
};
147-
flow.getVbar().addEventFilter(MouseEvent.MOUSE_PRESSED, ml);
148-
flow.getHbar().addEventFilter(MouseEvent.MOUSE_PRESSED, ml);
114+
lh.addEventFilter(flow.getVbar(), MouseEvent.MOUSE_PRESSED, ml);
115+
lh.addEventFilter(flow.getHbar(), MouseEvent.MOUSE_PRESSED, ml);
149116

150117
// init the behavior 'closures'
151118
behavior.setOnFocusPreviousRow(() -> onFocusAboveCell());
@@ -161,13 +128,13 @@ public TreeTableViewSkin(final TreeTableView<T> control) {
161128
behavior.setOnFocusLeftCell(() -> onFocusLeftCell());
162129
behavior.setOnFocusRightCell(() -> onFocusRightCell());
163130

164-
registerChangeListener(control.rootProperty(), e -> {
131+
lh.addChangeListener(control.rootProperty(), (ev) -> {
165132
// fix for RT-37853
166133
getSkinnable().edit(-1, null);
167-
168134
setRoot(getSkinnable().getRoot());
169135
});
170-
registerChangeListener(control.showRootProperty(), e -> {
136+
137+
lh.addChangeListener(control.showRootProperty(), (ev) -> {
171138
// if we turn off showing the root, then we must ensure the root
172139
// is expanded - otherwise we end up with no visible items in
173140
// the tree.
@@ -177,11 +144,19 @@ public TreeTableViewSkin(final TreeTableView<T> control) {
177144
// update the item count in the flow and behavior instances
178145
updateItemCount();
179146
});
180-
registerChangeListener(control.rowFactoryProperty(), e -> flow.recreateCells());
181-
registerChangeListener(control.expandedItemCountProperty(), e -> markItemCountDirty());
182-
registerChangeListener(control.fixedCellSizeProperty(), e -> flow.setFixedCellSize(getSkinnable().getFixedCellSize()));
183-
}
184147

148+
lh.addChangeListener(control.rowFactoryProperty(), (ev) -> {
149+
flow.recreateCells();
150+
});
151+
152+
lh.addChangeListener(control.expandedItemCountProperty(), (ev) -> {
153+
markItemCountDirty();
154+
});
155+
156+
lh.addChangeListener(control.fixedCellSizeProperty(), (ev) -> {
157+
flow.setFixedCellSize(getSkinnable().getFixedCellSize());
158+
});
159+
}
185160

186161

187162
/* *************************************************************************
@@ -191,12 +166,20 @@ public TreeTableViewSkin(final TreeTableView<T> control) {
191166
**************************************************************************/
192167

193168
/** {@inheritDoc} */
194-
@Override public void dispose() {
195-
super.dispose();
169+
@Override
170+
public void dispose() {
171+
flow.setCellFactory(null);
172+
173+
if (rootListener != null) {
174+
rootListener.disconnect();
175+
rootListener = null;
176+
}
196177

197178
if (behavior != null) {
198179
behavior.dispose();
199180
}
181+
182+
super.dispose();
200183
}
201184

202185
/** {@inheritDoc} */
@@ -299,13 +282,43 @@ private TreeItem<T> getRoot() {
299282
return weakRootRef == null ? null : weakRootRef.get();
300283
}
301284
private void setRoot(TreeItem<T> newRoot) {
302-
if (getRoot() != null && weakRootListener != null) {
303-
getRoot().removeEventHandler(TreeItem.<T>treeNotificationEvent(), weakRootListener);
285+
if (rootListener != null) {
286+
rootListener.disconnect();
287+
rootListener = null;
304288
}
305289
weakRootRef = new WeakReference<>(newRoot);
306290
if (getRoot() != null) {
307-
weakRootListener = new WeakEventHandler<>(rootListener);
308-
getRoot().addEventHandler(TreeItem.<T>treeNotificationEvent(), weakRootListener);
291+
// TODO I wonder if it might be possible for the root ref to get collected between these two lines
292+
// which would throw an NPE. Perhaps we should simply use newRoot instance instead of getRoot().
293+
rootListener = ListenerHelper.get(this).addEventHandler(getRoot(), TreeItem.<T>treeNotificationEvent(), e -> {
294+
if (e.wasAdded() && e.wasRemoved() && e.getAddedSize() == e.getRemovedSize()) {
295+
// Fix for RT-14842, where the children of a TreeItem were changing,
296+
// but because the overall item count was staying the same, there was
297+
// no event being fired to the skin to be informed that the items
298+
// had changed. So, here we just watch for the case where the number
299+
// of items being added is equal to the number of items being removed.
300+
markItemCountDirty();
301+
getSkinnable().requestLayout();
302+
} else if (e.getEventType().equals(TreeItem.valueChangedEvent())) {
303+
// Fix for RT-14971 and RT-15338.
304+
requestRebuildCells();
305+
} else {
306+
// Fix for RT-20090. We are checking to see if the event coming
307+
// from the TreeItem root is an event where the count has changed.
308+
EventType<?> eventType = e.getEventType();
309+
while (eventType != null) {
310+
if (eventType.equals(TreeItem.<T>expandedItemCountChangeEvent())) {
311+
markItemCountDirty();
312+
getSkinnable().requestLayout();
313+
break;
314+
}
315+
eventType = eventType.getSuperType();
316+
}
317+
}
318+
319+
// fix for RT-37853
320+
getSkinnable().edit(-1, null);
321+
});
309322
}
310323

311324
updateItemCount();

0 commit comments

Comments
 (0)