RFR: 8295754: PaginationSkin: memory leak when changing skin

[Andy Goryachev]

On Wed, 23 Nov 2022 00:13:07 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Fixes memory leaks as determined by SkinMemoryLeakTest (remove line 171) and a leak tester
>> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java
>> 
>> Make sure to configure the current test in LeakTest:
>> protected final Type WE_ARE_TESTING = Type.PAGINATION;
>> 
>> Found another issue: Pagination class does not survive replacing its skin (all components disappear).
>> 
>> caused by:
>> - adding and not removing listeners
>> - adding and not removing children Nodes
>> - setting control's properties in the constructor
>> - incorrectly setting a clip rectangle
>> 
>> NOTE: the fix will requires both ListenerHelper [JDK-8294809](https://bugs.openjdk.org/browse/JDK-8294809) and Skin.install() [JDK-8290844](https://bugs.openjdk.org/browse/JDK-8290844) changes.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java line 209:
> 
>> 207:         });
>> 208: 
>> 209:         ListenerHelper.get(this).addChangeListener(getSkinnable().heightProperty(), true, (ev) -> {
> 
> Why does `fireImmediately` need to be true?

this was needed to fix an issue found in PaginationSkin:
Pagination class does not survive replacing its skin (all components disappear).

> modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java line 1361:
> 
>> 1359:             // FIX own listener helper?
>> 1360:             //getSkinnable().getStyleClass().removeListener(updateSkinIndicatorType);
>> 1361:             //tooltipVisibleProperty().removeListener(updateTooltipVisibility);
> 
> I don't quite understand the comment. What follow-up is needed?

this method can be removed now.

> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java line 171:
> 
>> 169:                 MenuBar.class,
>> 170:                 MenuButton.class,
>> 171:                 //Pagination.class,
> 
> I would just remove this (which was done for other classes once the leaks were fixed).

Keeping this to avoid merge conflicts.  Will remove the whole exclusion block when the last skin is fixed.

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

PR: https://git.openjdk.org/jfx/pull/925