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

[John Hendrikx]

On Fri, 12 May 2023 15:52:56 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 103:
>> 
>>> 101:             updateTreeItem();
>>> 102:             // There used to be an isDirty = true statement here, but this was
>>> 103:             // determined to be unnecessary and led to performance issues such as
>> 
>> These changes don't fail the test when I undo them.  As said before, they're unnecessary.
>> 
>> Reasoning: they register on `TreeTableRow`, which is associated with the skin directly. If their lifecycles don't match, then `dispose` will take care of unregistering.  If their lifecycles do match, then they go out of scope at the same time.
>> 
>> Unless you prefer using the `register` functions, I think this change should be undone.
>
> Even though I don't particularly like register*Listener() because of its asymmetric nature (when it comes to removing listeners), here we do need to create weak listeners that get unregistered upon `dispose()`.  We need weak listeners because TreeTableView does not explicitly "disconnect" unused cells (e.g. refresh()), and we need `dispose()` due to skin life cycle.
> 
> So, in this particular case, I think this change should be ok.

Sorry, I think we may be misunderstanding each other.  I'm specifically talking about the two listeners added in `TreeTableRowSkin`'s constructor on `indexProperty` and `treeItemProperty`.  These are part of the `TreeTableRow` which is also discarded when `refresh` is called.

1) The test passes if these changes are reverted. This is a clear indication that either the test is insufficient, or that your assumption, that these must be weak, is incorrect. If you can construct a test that requires these listeners to be weak, then I think the changes are warranted.

2) At the risk of stating the obvious, the listeners in the constructor added using `ListenerHelper` are also correctly removed in `dispose`. `ListenerHelper` does this in a similar way to the `register` methods.

3) The `indexProperty` and `treeItemProperty` listeners are attached to the `TreeTableRow`, not the `TreeTableView`.  The refresh function replaces the entire row, including the skin. There is no need to use weak listeners for this purpose.  Compare this to the listeners that are attached to the `TreeTableView` or the `VirtualFlow`; these have a much longer lifecycle and can survive multiple refreshes and row factory replacements, and hence should be weak (for now).

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

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