RFR: 8242621: TabPane: Memory leak when switching skin [v3]

[Jeanette Winzenburg]

On Mon, 21 Dec 2020 11:54:06 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> `TabPaneSkin` installs some listeners that are not removed when `TabPaneSkin` is changed.
>> The fix converts listeners to WeakListeners and also removes them on dispose.
>> 
>> There is a NPE check change needed in `isHosrizontal()`. Without this check it causes NPE if pulse is in progress while TabPaneSkin is getting disposed.
>> 
>> `SkinMemoryLeakTest` already had a test which only needed to be enabled. 
>> Test fails before and passes after this change.
>
> Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review update

Fix looks good, verified tests failing (except one, commented) before and passing after the fix.

Left a couple of comments inline. Plus there's an @Ignore related to the issue in TabPaneTest (`testNPEOnSwitchSkinAndChangeSelection`) that should be removed.

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

> 1237:             }
> 1238:             controlButtons.removeListeners();
> 1239:         }

would suggest to rename the method to dispose - even though this here indeed does nothing but removing listeners (future changes might need to do additional cleanup).  That would be a  clear indication of its responsibility, just the same name as the cleanup method you added for TabContentRegion. If we decide to go for the rename, it should be done for all occurences of removeListeners in the other internal panes.

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

> 1575:             listener.dispose();
> 1576:             inner.getChildren().clear();
> 1577:             getChildren().clear();

I'm aware this is internal and old api but .. could we consider cleaning it up a bit?

- its task is to cleanup itself, doing whatever it deems is needed (here: removing listeners and children), so removeListeners is a misnomer as it describes only part of it. A (well-established) name for the task would be dispose
- passing in the tab is .. strange, as can be seen in its usage in TabHeaderArea.dispose:  `header.removeListeners(header.getTab())` Actually, it would be an error to call the method with a `tab != header.getTab()`

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

> 1733:             removeListeners();
> 1734:         }
> 1735: 

Wondering why the removal of children down the hierarchy? Shouldn't that be unnecessary as long as the direct children of the skin/control are and all listeners of the grandChildren to the skin are removed?

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

> 1844:             getSkinnable().sideProperty().removeListener(weakSidePropListener);
> 1845:             getSkinnable().getTabs().removeListener(weakTabsListenerForPopup);
> 1846:         }

yet another candidate for rename to dispose (see above)

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 275:

> 273:       assertEquals(3, tabPane.getChildrenUnmodifiable().size());
> 274:     }
> 275: 

hmm .. looks a bit unusual (could be me, though :)

- why showControl? just install/replace the skin should be enough?
- the outcome of testing an illegal state (after instantiating the skin and before really setting it as the control's skin at which time we have a skin : control relation of 2:1) is unspecified. F.i. there are skins that replace all children in the constructor (which may or may not be a good idea) that would fail a test similar to this.

How about

    TabPane tabPane ..
    installDefaultSkin(tabPane);
    int children = tabPane.getxxChildren().size();
    replaceSkin(tabPane);
    assertEquals(children, tabPane.getxxChildren().size());

If we decide to go without showing, the other tests should be changed as well :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 289:

> 287:       assertNull(tabPane.getSkin());
> 288:       assertEquals(0, tabPane.getChildrenUnmodifiable().size());
> 289:     }

Same as above: what's the reason to actually show?

And just curious:  why is this passing already before the fix?

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 302:

> 300:     @Test
> 301:     public void testNPEWhenRemTabAfterSkinIsReplaced() {
> 302:       TabPane tabPane = new TabPane();

Personally I don't like abbreviation in method names, not even test methods. `testNPEWhenRemoveTabAfterSkinIsReplaced` isn't that much longer :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 311:

> 309:     @Test
> 310:     public void testAddRemTabsAfterSkinIsReplaced() {
> 311:       TabPane tabPane = new TabPane();

see above

-------------

Changes requested by fastegal (Committer).

PR: https://git.openjdk.java.net/jfx/pull/318