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

[Kevin Rushforth]

On Wed, 10 May 2023 15:20:25 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

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

Even if you are using weak listeners, `dispose` should remove them.

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

If it's easy to add another test for additional missing cases that would provide better coverage for your fix, it seems worth doing.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1190136063
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1190138025