On Sat, 16 Nov 2024 00:58:47 GMT, Marius Hanl <mh...@openjdk.org> wrote:
> There are multiple issues in the `TableMenu` logic that result in a memory > leak. > > The following problems are now fixed with this PR: > - The listener, that is registered to the `col.visibleProperty()` was not > weak nor was it ever unregistered > - The fix is to do pretty much the same that was already done with the > `col.textProperty()` listener > - The `col.visibleProperty()` and `col.textProperty()` where added again and > again and again to the column when the columns changed (which happens when > toggling the visibility). > - The fix is to add the listeners once - when the `MenuItem` is created. > This way, when the `TableMenu` is rebuild and the cached `MenuItem` is used, > the whole listener logic is not run again for the column modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableHeaderRow.java line 641: > 639: > 640: final CheckMenuItem _item = item; > 641: // fake bidrectional binding (a real one was used here but > resulted in JBS-8136468) that's JDK-8136468 modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableViewTableHeaderRowTest.java line 233: > 231: /** > 232: * Tests that re-setting the same columns does not cause memory > leaks. > 233: * See also: <a > href="https://bugs.openjdk.org/browse/JDK-8341687";>JDK-8341687</a>. technically, this test does not test for memory leak - it seems to test the fact that the listener gets disconnected. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableViewTableHeaderRowTest.java line 261: > 259: /** > 260: * Tests that toggling the column visibility does not cause memory > leaks. > 261: * See also: <a > href="https://bugs.openjdk.org/browse/JDK-8341687";>JDK-8341687</a>. same comment - does not test for memory leak. the memory leak test might involve `JMemoryBuddy` - see for example `SystemMenuBarTest`. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableViewTableHeaderRowTest.java line 234: > 232: /** > 233: * Tests that re-setting the same columns does not cause memory > leaks. > 234: * See also: <a > href="https://bugs.openjdk.org/browse/JDK-8341687";>JDK-8341687</a>. same, does not test for leaks. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1640#discussion_r1846878710 PR Review Comment: https://git.openjdk.org/jfx/pull/1640#discussion_r1846882432 PR Review Comment: https://git.openjdk.org/jfx/pull/1640#discussion_r1846887787 PR Review Comment: https://git.openjdk.org/jfx/pull/1640#discussion_r1846889065
