RFR: 8307538: Memory leak in TreeTableView when calling refresh [v2]

[John Hendrikx]

On Mon, 8 May 2023 18:55:38 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Fixed a memory leak in TreeTableView by using WeakInvalidationListener (which is the right approach in this particular situation) - the leak is specific to TreeTableRowSkin.
>> 
>> Added a unit test.
>> 
>> Added Refresh buttons and Skin menu to the Monkey Tester (will expand these to other controls as a part next MT update).
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
> 
>  - removed monkey tester changes
>  - Merge remote-tracking branch 'origin/master' into 8307538.refresh
>  - whitespace
>  - updated test
>  - variable tree cell fix
>  - fixed size selector
>  - list view page
>  - fix and test
>  - skin menu
>  - refresh button

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 101:

> 99:         control.indexProperty().addListener(new WeakInvalidationListener((x) -> {
> 100:             updateCells = true;
> 101:         }));

I don't think this needed changing, the index property is directly available on `TreeTableRow` and so shouldn't cause GC issues.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 108:

> 106:             // determined to be unnecessary and led to performance issues such as
> 107:             // those detailed in JDK-8143266
> 108:         }));

As above, I don't think this needs to be weak.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 334:

> 332:     private void updateTreeItem() {
> 333:         unregisterInvalidationListeners(graphicProperty());
> 334:         treeItem = (getSkinnable() == null) ? null : getSkinnable().getTreeItem();

This is not needed if the weak listener change is reverted in the constructor. The cause for it being `null` is that the listener is now no longer removed by `dispose` where before (with `ListenerHelper` or `registerXXXListener`) it would have been.

Also, I think when `dispose` is called, this skin should still do its utmost best to remove all listeners immediately.

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java line 335:

> 333: 
> 334:         treeTableView.refresh();
> 335:         Toolkit.getToolkit().firePulse();

I think cells should also be collectable in other situations, like replacing the row factory.  Is it worth adding a test for that?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189429842
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189430267
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189434672
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189438781