Skip to content

Commit

Permalink
8242621: TabPane: Memory leak when switching skin
Browse files Browse the repository at this point in the history 
Reviewed-by: fastegal, kcr
  • Loading branch information
@arapte
arapte committed Jan 7, 2021
1 parent e74f679 commit c197b62
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 25 deletions.
77 changes: 56 additions & 21 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
View file
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
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
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
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

0 comments on commit c197b62

Please sign in to comment.