On Mon, 8 May 2023 18:55:38 GMT, Andy Goryachev <ango...@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 I've seen the problem with Views and Cell creation often enough now that I think we may want to talk about the fundamental problem that is causing these issues. The lifecycle of a Cell or Row is not the same as the View that contains them. Cells are created on demand, and should either be destroyed or unlinked. In JavaFX, we have chosen to not have an explicit `destroy` method for cells, but we do have a way to unlink cells. For a `TreeTableRow` this is a simple as calling `TreeTableRow#updateTreeTableView(null)`. The call already does exactly what you would expect: it unregisters all troublesome listeners (ones that were registered on the view). The skin also responds to this setting to `null` as it adds listeners to the `treeTableViewProperty` of its skinnable. In other words, a `TreeTableView` which creates cells on demand with a different lifecycle than itself should also be responsible to clean them up when it no longer needs them -- this is where the real bug is. It is managing the lifecycle of these cells and should not rely on these cells doing this indirectly by using a lot of weak listeners. If `TreeTableView` would correctly unlink cells and rows it doesn't need, it would remove the need for **all** weak listeners (and also references) in both `TreeTableRow` and `TreeTableRowSkin`, making their code much simpler, more predictable and easier to test. Specifically for this PR, only the listeners registered as part of `setupTreeTableViewListeners` need to be weak, the others are not the issue. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1129#issuecomment-1538921567
