Skip to content

Commit

Permalink
8307538: Memory leak in TreeTableView when calling refresh
Browse files Browse the repository at this point in the history 
Reviewed-by: kcr, aghaisas, mhanl
  • Loading branch information
Andy Goryachev committed Jun 6, 2023
1 parent 05548ac commit 17ed2e7
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 22 deletions.
39 changes: 24 additions & 15 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2023, 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 @@ -28,7 +28,6 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import javafx.beans.property.DoubleProperty;
import javafx.beans.property.ObjectProperty;
import javafx.collections.FXCollections;
Expand All @@ -48,7 +47,6 @@
import javafx.scene.control.TreeTablePosition;
import javafx.scene.control.TreeTableRow;
import javafx.scene.control.TreeTableView;

import com.sun.javafx.scene.control.ListenerHelper;
import com.sun.javafx.scene.control.behavior.BehaviorBase;
import com.sun.javafx.scene.control.behavior.TreeTableRowBehavior;
Expand All @@ -75,8 +73,6 @@ public class TreeTableRowSkin<T> extends TableRowSkinBase<TreeItem<T>, TreeTable
private final BehaviorBase<TreeTableRow<T>> behavior;
private boolean childrenDirty = false;



/* *************************************************************************
* *
* Constructors *
Expand Down Expand Up @@ -116,43 +112,56 @@ public TreeTableRowSkin(TreeTableRow<T> control) {

// FIXME: replace listener to fixedCellSize with direct lookup - JDK-8277000
private void setupTreeTableViewListeners() {
ListenerHelper lh = ListenerHelper.get(this);
TreeTableView<T> treeTableView = getSkinnable().getTreeTableView();
if (treeTableView == null) {
lh.addInvalidationListener(getSkinnable().treeTableViewProperty(), (ev) -> {
registerInvalidationListener(getSkinnable().treeTableViewProperty(), (x) -> {
unregisterInvalidationListeners(getSkinnable().treeTableViewProperty());
setupTreeTableViewListeners();
});
} else {
lh.addChangeListener(treeTableView.treeColumnProperty(), (ev) -> {
registerChangeListener(treeTableView.treeColumnProperty(), (x) -> {
// Fix for RT-27782: Need to set isDirty to true, rather than the
// cheaper updateCells, as otherwise the text indentation will not
// be recalculated in TreeTableCellSkin.calculateIndentation()
isDirty = true;
getSkinnable().requestLayout();
if (getSkinnable() != null) {
getSkinnable().requestLayout();
}
});

DoubleProperty fixedCellSizeProperty = getTreeTableView().fixedCellSizeProperty();
if (fixedCellSizeProperty != null) {
lh.addChangeListener(fixedCellSizeProperty, (ev) -> {
fixedCellSize = fixedCellSizeProperty.get();
fixedCellSizeEnabled = fixedCellSize > 0;
registerChangeListener(fixedCellSizeProperty, (x) -> {
updateCachedFixedSize();
});
fixedCellSize = fixedCellSizeProperty.get();
fixedCellSizeEnabled = fixedCellSize > 0;
updateCachedFixedSize();

// JDK-8144500:
// When in fixed cell size mode, we must listen to the width of the virtual flow, so
// that when it changes, we can appropriately add / remove cells that may or may not
// be required (because we remove all cells that are not visible).
VirtualFlow<TreeTableRow<T>> virtualFlow = getVirtualFlow();
if (virtualFlow != null) {
lh.addChangeListener(getVirtualFlow().widthProperty(), (ev) -> treeTableView.requestLayout());
registerChangeListener(getVirtualFlow().widthProperty(), (x) -> {
if (getSkinnable() != null) {
TreeTableView<T> t = getSkinnable().getTreeTableView();
t.requestLayout();
}
});
}
}
}
}

private void updateCachedFixedSize() {
if (getSkinnable() != null) {
TreeTableView<T> t = getSkinnable().getTreeTableView();
if (t != null) {
fixedCellSize = t.getFixedCellSize();
fixedCellSizeEnabled = fixedCellSize > 0.0;
}
}
}

/* *************************************************************************
* *
Expand Down
50 changes: 43 additions & 7 deletions ...es/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@

package test.javafx.scene.control.skin;

import com.sun.javafx.tk.Toolkit;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.lang.ref.WeakReference;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.geometry.Insets;
Expand All @@ -44,14 +48,11 @@
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import com.sun.javafx.tk.Toolkit;
import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
import test.com.sun.javafx.scene.control.test.Person;

import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertEquals;
import test.util.memory.JMemoryBuddy;

public class TreeTableRowSkinTest {

Expand Down Expand Up @@ -212,10 +213,12 @@ public void treeTableRowWithFixedCellSizeShouldIgnoreVerticalPadding() {
int right = 20;
int bottom = 30;
int left = 40;
double fixed = 24;

int horizontalPadding = left + right;

treeTableView.setFixedCellSize(24);
treeTableView.setFixedCellSize(fixed);
Toolkit.getToolkit().firePulse();

IndexedCell cell = VirtualFlowTestUtils.getCell(treeTableView, 0);

Expand All @@ -225,6 +228,8 @@ public void treeTableRowWithFixedCellSizeShouldIgnoreVerticalPadding() {
double width = cell.getWidth();

double minHeight = cell.minHeight(-1);
assertEquals(fixed, minHeight, 0);

double prefHeight = cell.prefHeight(-1);
double maxHeight = cell.maxHeight(-1);
double height = cell.getHeight();
Expand All @@ -238,6 +243,7 @@ public void treeTableRowWithFixedCellSizeShouldIgnoreVerticalPadding() {
treeTableView.refresh();
Toolkit.getToolkit().firePulse();

assertNotEquals(cell, VirtualFlowTestUtils.getCell(treeTableView, 0));
cell = VirtualFlowTestUtils.getCell(treeTableView, 0);

assertEquals(minWidth + horizontalPadding, cell.minWidth(-1), 0);
Expand Down Expand Up @@ -320,6 +326,36 @@ public void invisibleColumnsShouldRemoveCorrespondingCellsInRow() {
invisibleColumnsShouldRemoveCorrespondingCellsInRowImpl();
}

/** TreeTableView.refresh() must release all discarded cells JDK-8307538 */
@Test
public void cellsMustBeCollectableAfterRefresh() {
IndexedCell<?> row = VirtualFlowTestUtils.getCell(treeTableView, 0);
assertNotNull(row);
WeakReference<Object> ref = new WeakReference<>(row);
row = null;

treeTableView.refresh();
Toolkit.getToolkit().firePulse();

JMemoryBuddy.assertCollectable(ref);
}

/** TreeTableView.setRowFactory() must release all discarded cells JDK-8307538 */
@Test
public void cellsMustBeCollectableAfterRowFactoryChange() {
IndexedCell<?> row = VirtualFlowTestUtils.getCell(treeTableView, 0);
assertNotNull(row);
WeakReference<Object> ref = new WeakReference<>(row);
row = null;

treeTableView.setRowFactory((x) -> {
return new TreeTableRow<>();
});
Toolkit.getToolkit().firePulse();

JMemoryBuddy.assertCollectable(ref);
}

@AfterEach
public void after() {
stageLoader.dispose();
Expand Down

1 comment on commit 17ed2e7

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

Please sign in to comment.