8241737: TabPaneSkin memory leak on replacing selectionModel

Reviewed-by: fastegal, kcr

Author: arapte

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

@@ -69,6 +69,7 @@
 import javafx.scene.control.Label;
 import javafx.scene.control.MenuItem;
 import javafx.scene.control.RadioMenuItem;
+import javafx.scene.control.SelectionModel;
 import javafx.scene.control.SkinBase;
 import javafx.scene.control.Tab;
 import javafx.scene.control.TabPane;
@@ -195,12 +196,9 @@ public TabPaneSkin(TabPane control) {
         }
 
         initializeTabListener();
+        updateSelectionModel();
 
-        registerChangeListener(control.getSelectionModel().selectedItemProperty(), e -> {
-            isSelectingTab = true;
-            selectedTab = getSkinnable().getSelectionModel().getSelectedItem();
-            getSkinnable().requestLayout();
-        });
+        registerChangeListener(control.selectionModelProperty(), e -> updateSelectionModel());
         registerChangeListener(control.sideProperty(), e -> updateTabPosition());
         registerChangeListener(control.widthProperty(), e -> clipRect.setWidth(getSkinnable().getWidth()));
         registerChangeListener(control.heightProperty(), e -> clipRect.setHeight(getSkinnable().getHeight()));
@@ -257,8 +255,6 @@ public TabPaneSkin(TabPane control) {
         }
     };
 
-
-
     /***************************************************************************
      *                                                                         *
      * Public API                                                              *
@@ -267,6 +263,11 @@ public TabPaneSkin(TabPane control) {
 
     /** {@inheritDoc} */
     @Override public void dispose() {
+        if (selectionModel != null) {
+            selectionModel.selectedItemProperty().removeListener(weakSelectionChangeListener);
+            selectionModel = null;
+        }
+
         super.dispose();
 
         if (behavior != null) {
@@ -429,6 +430,25 @@ public TabPaneSkin(TabPane control) {
      *                                                                         *
      **************************************************************************/
 
+    private SelectionModel<Tab> selectionModel;
+    private InvalidationListener selectionChangeListener = observable -> {
+        isSelectingTab = true;
+        selectedTab = getSkinnable().getSelectionModel().getSelectedItem();
+        getSkinnable().requestLayout();
+    };
+    private WeakInvalidationListener weakSelectionChangeListener =
+            new WeakInvalidationListener(selectionChangeListener);
+
+    private void updateSelectionModel() {
+        if (selectionModel != null) {
+            selectionModel.selectedItemProperty().removeListener(weakSelectionChangeListener);
+        }
+        selectionModel = getSkinnable().getSelectionModel();
+        if (selectionModel != null) {
+            selectionModel.selectedItemProperty().addListener(weakSelectionChangeListener);
+        }
+    }
+
     private static int getRotation(Side pos) {
         switch (pos) {
             case TOP:

modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java

@@ -194,9 +194,6 @@ public void testListViewSelectionModel() {
 
     @Test
     public void testTabPaneSelectionModel() {
-        // FIXME
-        // can't formally ignore just one parameter, so backing out if showBeforeReplaceSM
-        if (showBeforeReplaceSM) return; //@Ignore("8241737")
         TabPane control = new TabPane();
         ObservableList<String> data = FXCollections.observableArrayList("Apple", "Orange", "Banana");
         data.forEach(text -> control.getTabs().add(new Tab(text)));

modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java

@@ -37,6 +37,8 @@
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import javafx.scene.control.SelectionModel;
+import javafx.scene.control.Skin;
 import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
 import javafx.application.Platform;
 import javafx.beans.property.BooleanProperty;
@@ -1155,27 +1157,26 @@ public void test_rt_38382(boolean addToTabPane) {
     }
 
     // Test for JDK-8154039
-    WeakReference<Tab> weakTab;
     @Test public void testSelectNonChildTab() {
         tabPane.getTabs().addAll(tab1);
         root.getChildren().add(tabPane);
         show();
         tk.firePulse();
-        weakTab = new WeakReference<>(new Tab("NonChildTab"));
+        WeakReference<Tab> weakTab = new WeakReference<>(new Tab("NonChildTab"));
         tabPane.getSelectionModel().select(weakTab.get());
         tk.firePulse();
-        attemptGC(10);
+        attemptGC(10, weakTab);
         tk.firePulse();
         assertNull(weakTab.get());
     }
 
-    private void attemptGC(int n) {
+    private void attemptGC(int n, WeakReference<?> weakRef) {
         // Attempt gc n times
         for (int i = 0; i < n; i++) {
             System.gc();
             System.runFinalization();
 
-            if (weakTab.get() == null) {
+            if (weakRef.get() == null) {
                 break;
             }
             try {
@@ -1207,4 +1208,41 @@ private void attemptGC(int n) {
     private int sortCompare(Tab t1, Tab t2) {
         return t2.getText().compareTo(t1.getText());
     }
+
+    class TabPaneSkin1 extends TabPaneSkin {
+        TabPaneSkin1(TabPane tabPane) {
+            super(tabPane);
+        }
+    }
+
+    @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();
+        tk.firePulse();
+
+        tabPane.setSkin(new TabPaneSkin1(tabPane));
+        tk.firePulse();
+        tabPane.getSelectionModel().select(1);
+        tk.firePulse();
+    }
+
+    @Test
+    public void testSMLeakOnSwitchSkinAndSM() {
+        tabPane.getTabs().addAll(tab1, tab2);
+        root.getChildren().add(tabPane);
+        stage.show();
+        tk.firePulse();
+
+        WeakReference<SelectionModel<Tab>> weakSMRef = new WeakReference<>(tabPane.getSelectionModel());
+        tabPane.setSkin(new TabPaneSkin1(tabPane));
+        tk.firePulse();
+        tabPane.setSelectionModel(TabPaneShim.getTabPaneSelectionModel(tabPane));
+        tk.firePulse();
+        attemptGC(10, weakSMRef);
+        assertNull(weakSMRef.get());
+    }
 }