On Mon, 21 Dec 2020 12:06:16 GMT, Ambarish Rapte <[email protected]> wrote:
>> 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. quick nit-pick: > This does not stop a skin object from getting GCed, actually, it does ... if it keeps a strong reference to the skin :) ------------- PR: https://git.openjdk.java.net/jfx/pull/318
