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

[Andy Goryachev]

On Wed, 10 May 2023 06:54:35 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

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

The problem is, `dispose()` is never called for cells discarded by the refresh() method.

In the absence of explicit release, these cells are still listening - so using a weak listener in this case is, I think, the right approach.  Adding explicit release per your (absolutely valid) suggestion is a good idea, but much, much more complicated task.

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

this test reflects the exact failure scenario described in the ticket.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1190070346
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1190071689