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

[Andy Goryachev]

On Fri, 12 May 2023 18:13:46 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

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

you are right, of course.  and I should know - ListenerHelper is my baby.  I guess the fear of another regression overtook me.

reverting.

thank you for your patience and persistence!

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

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