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

[Ambarish Rapte]

On Tue, 15 Dec 2020 14:56:09 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> 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.

> As mentioned offline, can you check whether removing `tabHeaderArea` is necessary?

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


The list returned by `SkinBase.getChildren()` is actually the same list as `Parent.getChildren()`.
`SkinBase()` constructor gets the reference of `Parent.getChildren()` and store it's reference in the class member named `children`. [[see here](https://github.com/openjdk/jfx/blob/53fe38bb34fc01ba298aa1a12f20c061c0cb58b2/modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java#L125)]
So, Skin and Parent share the same list of children. So when a skin is being `dispose()`ed, it should remove any children that it added to the list. Otherwise, those children continue to be part of scenegraph.
Please check the newly added test SkinCleanupTest.testChildrenCountAfterSkinIsReplaced().

This does not stop a skin object from getting GCed, but the Children that skin adds don't get removed from Scenegraph. Looking at this behavior, it seems like this should be done for cleanup of all Skins.

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

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