RFR: 8295754: PaginationSkin: memory leak when changing skin

[Kevin Rushforth]

On Thu, 20 Oct 2022 20:48:07 GMT, Andy Goryachev <angorya 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.

I looked at this in connection with PR #908 . It generally looks good. I left a few questions.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java line 201:

> 199:         getChildren().addAll(currentStackPane, nextStackPane, navigation);
> 200: 
> 201:         ListenerHelper.get(this).addInvalidationListener(getSkinnable().maxPageIndicatorCountProperty(), (o) -> {

Minor: In the constructor, I recommend using the `control` argument rather than calling `getSkinnable()`, although this is just a recommendation.

Minor: since you make several calls to `ListenerHelper`, a temporary variable might make the code simpler; this is also just a recommendation.

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?

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?

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

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

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