Skip to content

8242621: TabPane: Memory leak when switching skin #318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
arapte wants to merge 5 commits into from
Closed

8242621: TabPane: Memory leak when switching skin #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
47674a6
8242621: TabPane: Memory leak when switching skin
arapte Oct 13, 2020
cb7f9ee
Merge branch 'master' into TabPaneSkinLeak
arapte Dec 21, 2020
a3b5795
Review update
arapte Dec 21, 2020
cc16aa5
Review update: cleanup and corrections
arapte Jan 6, 2021
93e1645
Space corrections in tests
arapte Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 56 additions & 21 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,10 +267,22 @@ public TabPaneSkin(TabPane control) {

/** {@inheritDoc} */
@Override public void dispose() {
if (getSkinnable() == null) return;

if (selectionModel != null) {
selectionModel.selectedItemProperty().removeListener(weakSelectionChangeListener);
selectionModel = null;
}
getSkinnable().getTabs().removeListener(weakTabsListener);
tabHeaderArea.dispose();

// Control and Skin share the list of children, so children that are
// added by Skin are actually added to control's list of children,
// so a skin should remove the children that it adds.
getChildren().remove(tabHeaderArea);
for (Tab tab : getSkinnable().getTabs()) {
removeTabContent(tab);
}

super.dispose();

Expand Down Expand Up @@ -499,7 +511,7 @@ private void removeTabs(List<? extends Tab> removedList) {
if (tabRegion != null) {
tabRegion.isClosing = true;

tabRegion.removeListeners(tab);
tabRegion.dispose();
removeTabContent(tab);

EventHandler<ActionEvent> cleanup = ae -> {
Expand Down Expand Up @@ -579,8 +591,10 @@ private void addTabs(List<? extends Tab> addedList, int from) {
}
}

ListChangeListener<Tab> tabsListener;
WeakListChangeListener<Tab> weakTabsListener;
private void initializeTabListener() {
getSkinnable().getTabs().addListener((ListChangeListener<Tab>) c -> {
tabsListener = c -> {
List<Tab> tabsToRemove = new ArrayList<>();
List<Tab> tabsToAdd = new ArrayList<>();

Expand Down Expand Up @@ -659,7 +673,9 @@ private void initializeTabListener() {

// Fix for RT-34692
getSkinnable().requestLayout();
});
};
weakTabsListener = new WeakListChangeListener<>(tabsListener);
getSkinnable().getTabs().addListener(weakTabsListener);
}

private void addTabContent(Tab tab) {
Expand All @@ -673,14 +689,18 @@ private void addTabContent(Tab tab) {
private void removeTabContent(Tab tab) {
for (TabContentRegion contentRegion : tabContentRegions) {
if (contentRegion.getTab().equals(tab)) {
contentRegion.removeListeners(tab);
getChildren().remove(contentRegion);
tabContentRegions.remove(contentRegion);
removeTabContent(contentRegion);
break;
}
}
}

private void removeTabContent(TabContentRegion contentRegion) {
contentRegion.dispose();
tabContentRegions.remove(contentRegion);
getChildren().remove(contentRegion);
}

private void updateTabPosition() {
tabHeaderArea.invalidateScrollOffset();
getSkinnable().applyCss();
Expand Down Expand Up @@ -1208,6 +1228,14 @@ private double firstTabIndent() {
positionInArea(controlButtons, controlStartX, controlStartY, btnWidth, btnHeight,
/*baseline ignored*/0, HPos.CENTER, VPos.CENTER);
}

void dispose() {
for (Node child : headersRegion.getChildren()) {
TabHeaderSkin header = (TabHeaderSkin) child;
header.dispose();
}
controlButtons.dispose();
}
} /* End TabHeaderArea */


Expand Down Expand Up @@ -1495,7 +1523,7 @@ public void handle(MouseEvent me) {
if (me.getButton().equals(MouseButton.MIDDLE)) {
if (showCloseButton()) {
if (behavior.canCloseTab(tab)) {
removeListeners(tab);
dispose();
behavior.closeTab(tab);
}
}
Expand Down Expand Up @@ -1541,10 +1569,9 @@ private boolean showCloseButton() {
}
};

private void removeListeners(Tab tab) {
private void dispose() {
tab.getStyleClass().removeListener(weakStyleClassListener);
listener.dispose();
inner.getChildren().clear();
getChildren().clear();
setOnContextMenuRequested(null);
setOnMousePressed(null);
}
Expand Down Expand Up @@ -1693,7 +1720,7 @@ private void updateContent() {
}
}

private void removeListeners(Tab tab) {
public void dispose() {
tab.selectedProperty().removeListener(weakTabSelectedListener);
tab.contentProperty().removeListener(weakTabContentListener);
}
Expand Down Expand Up @@ -1785,11 +1812,8 @@ private void positionArrow(Pane btn, StackPane arrow, double x, double y, double

getChildren().add(inner);

tabPane.sideProperty().addListener(valueModel -> {
Side tabPosition = getSkinnable().getSide();
downArrow.setRotate(tabPosition.equals(Side.BOTTOM)? 180.0F : 0.0F);
});
tabPane.getTabs().addListener((ListChangeListener<Tab>) c -> setupPopupMenu());
tabPane.sideProperty().addListener(weakSidePropListener);
tabPane.getTabs().addListener(weakTabsListenerForPopup);
showControlButtons = false;
if (isShowTabsMenu()) {
showControlButtons = true;
Expand All @@ -1798,6 +1822,21 @@ private void positionArrow(Pane btn, StackPane arrow, double x, double y, double
getProperties().put(ContextMenu.class, popup);
}

InvalidationListener sidePropListener = e -> {
Side tabPosition = getSkinnable().getSide();
downArrow.setRotate(tabPosition.equals(Side.BOTTOM)? 180.0F : 0.0F);
};
ListChangeListener<Tab> tabsListenerForPopup = e -> setupPopupMenu();
WeakInvalidationListener weakSidePropListener =
new WeakInvalidationListener(sidePropListener);
WeakListChangeListener weakTabsListenerForPopup =
new WeakListChangeListener<>(tabsListenerForPopup);

void dispose() {
getSkinnable().sideProperty().removeListener(weakSidePropListener);
getSkinnable().getTabs().removeListener(weakTabsListenerForPopup);
}

private boolean showTabsMenu = false;

private void showTabsMenu(boolean value) {
Expand Down Expand Up @@ -2058,11 +2097,7 @@ private void setupReordering(StackPane headersRegion) {
dragState = DragState.NONE;
this.headersRegion = headersRegion;
updateListeners();
getSkinnable().tabDragPolicyProperty().addListener((observable, oldValue, newValue) -> {
if (oldValue != newValue) {
updateListeners();
}
});
registerChangeListener(getSkinnable().tabDragPolicyProperty(), inv -> updateListeners());
}

private void handleHeaderMousePressed(MouseEvent event) {
Expand Down
2 changes: 0 additions & 2 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1213,10 +1213,8 @@ class TabPaneSkin1 extends TabPaneSkin {
}
}

@Ignore("JDK-8242621")
@Test
public void testNPEOnSwitchSkinAndChangeSelection() {
// Because of JDK-8242621, this test fails with NPE.
tabPane.getTabs().addAll(tab1, tab2);
root.getChildren().add(tabPane);
stage.show();
Expand Down
60 changes: 60 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,14 @@
import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;

import javafx.scene.Scene;
import javafx.scene.control.skin.TabPaneSkin;
import javafx.scene.control.Button;
import javafx.scene.control.ChoiceBox;
import javafx.scene.control.Control;
import javafx.scene.control.ListCell;
import javafx.scene.control.ListView;
import javafx.scene.control.Tab;
import javafx.scene.control.TabPane;
import javafx.scene.control.ToolBar;
import javafx.scene.control.TreeCell;
import javafx.scene.control.TreeItem;
Expand Down Expand Up @@ -257,6 +260,63 @@ public void testToolBarFocus() {
assertEquals("first item in toolbar must be focused", bar.getItems().get(0), scene.getFocusOwner());
}

//-------- TabPane
@Test
public void testChildrenCountAfterSkinIsReplaced() {
TabPane tabPane = new TabPane();
tabPane.getTabs().addAll(new Tab("0"), new Tab("1"));
installDefaultSkin(tabPane);
int childrenCount = tabPane.getChildrenUnmodifiable().size();
replaceSkin(tabPane);
assertEquals(childrenCount, tabPane.getChildrenUnmodifiable().size());
}

@Test
public void testChildrenCountAfterSkinIsRemoved() {
TabPane tabPane = new TabPane();
assertEquals(0, tabPane.getChildrenUnmodifiable().size());
tabPane.getTabs().addAll(new Tab("0"), new Tab("1"));
installDefaultSkin(tabPane);
assertEquals(3, tabPane.getChildrenUnmodifiable().size());
tabPane.setSkin(null);
assertNull(tabPane.getSkin());
assertEquals(0, tabPane.getChildrenUnmodifiable().size());
}

@Test
public void testNPEWhenTabsAddedAfterSkinIsReplaced() {
TabPane tabPane = new TabPane();
tabPane.getTabs().addAll(new Tab("0"), new Tab("1"));
installDefaultSkin(tabPane);
replaceSkin(tabPane);
tabPane.getTabs().addAll(new Tab("2"), new Tab("3"));
}

@Test
public void testNPEWhenTabRemovedAfterSkinIsReplaced() {
TabPane tabPane = new TabPane();
tabPane.getTabs().addAll(new Tab("0"), new Tab("1"));
installDefaultSkin(tabPane);
replaceSkin(tabPane);
tabPane.getTabs().remove(0);
}

@Test
public void testAddRemoveTabsAfterSkinIsReplaced() {
TabPane tabPane = new TabPane();
tabPane.getTabs().addAll(new Tab("0"), new Tab("1"));
installDefaultSkin(tabPane);
assertEquals(2, tabPane.getTabs().size());
assertEquals(3, tabPane.getChildrenUnmodifiable().size());
replaceSkin(tabPane);
tabPane.getTabs().addAll(new Tab("2"), new Tab("3"));
assertEquals(4, tabPane.getTabs().size());
assertEquals(5, tabPane.getChildrenUnmodifiable().size());
tabPane.getTabs().clear();
assertEquals(0, tabPane.getTabs().size());
assertEquals(1, tabPane.getChildrenUnmodifiable().size());
}

//---------------- setup and initial

/**
Expand Down
2 changes: 0 additions & 2 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,6 @@ public static Collection<Object[]> data() {
SplitPane.class,
TableRow.class,
TableView.class,
// @Ignore("8242621")
TabPane.class,
// @Ignore("8244419")
TextArea.class,
// @Ignore("8240506")
Expand Down