RFR: 8242621: TabPane: Memory leak when switching skin

[Jeanette Winzenburg]

On Thu, 15 Oct 2020 23:23:29 GMT, Kevin Rushforth <kcr 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.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 274:
> 
>> 272:         getSkinnable().getTabs().removeListener(weakTabsListener);
>> 273: 
>> 274:         getChildren().remove(tabHeaderArea);
> 
> As mentioned offline, can you check whether removing `tabHeaderArea` is necessary?

good question :) Just have run into a similar case while working on cleaning up TextFieldSkin.

My current understanding is ... depends ;) 

As tests show, some children keep the skin alive, others don't. Which must imply that the first somehow keep a strong reference to the skin, the latter (nor any of its children) don't.  An example for the former is tabHeaderArea, an example for the latter is tabContentRegion. Was puzzled about how to distinguish the one from the other (which boils down to finding that strong reference) - until I (faintly ;) remembered that inner classes have an implicit reference to the enclosing instance: TabHeaderArea is an inner class with an implicit reference to the enclosing skin, while TabContentRegion is static nested. As long as the former reside in the scenegraph, it makes the skin leaky.

So we have to watch out for 
- explicit strong references from any node (down the hierarchy) to the skin
- implicit strong reference to the enclosing skin class.

Both groups have to be removed in dispose to prevent memory leaks.  

Even if not obviously leaking, children pile up when switching skin: most skins add to children, rarely set. We started a discussion of how to handle those that add [Spinner misbehavior](https://bugs.openjdk.java.net/browse/JDK-8245145) - not yet decided (personally, I didn't do anything about them in the cleanups, deferred to later ;)  From today's knowledge, I would suggest to explicitly remove all direct children that the skin added to the control.

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

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