RFR: 8341687: Memory leak in TableView after interacting with TableMenuButton

[Andy Goryachev]

On Sat, 16 Nov 2024 00:58:47 GMT, Marius Hanl <mhanl at 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