RFR: 8318841: macOS: Memory leak with MenuItem when Menu.useSystemMenuBar(true) is used [v4]

[Kevin Rushforth]

On Mon, 6 Nov 2023 19:22:26 GMT, Johan Vos <jvos at openjdk.org> wrote:

>> When the Java layer removes a systemmenu, release the native resources related to this systemmenu.
>> This removes the strong JNI Global ref, which prevents its references from being gc'ed.
>> 
>> The current implementation for the mac-specific system menu creates a menu, but never releases its resources. In the `dealloc` of this menu, the strong jni refs are deleted.
>> With this PR, we now release the native resources associated with a menuItem when that one is removed from a menu. A consequence is that this menuItem should never be used after being removed from its current menu (e.g. it should not be re-added, or its text/shortcut should not be altered). 
>> The current implementation will create a new MacMenuDelegate every time a menuItem is inserted into a menu, so there should be no references to the native resources lingering.
>
> Johan Vos has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add a system test

The updated fix looks good. The test passes with the fix and hangs indefinitely without the fix. I left a comment about the fix for the hang and a couple other comments as well.

tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 122:

> 120:             Thread.currentThread().setUncaughtExceptionHandler((t,e) -> {
> 121:                 e.printStackTrace();
> 122:                 failed.set(true);

To avoid the test hanging indefinitely, I recommend adding the following after setting the `failed` flag:


    memoryLatch.countDown();

tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 145:

> 143:         stage.show();
> 144:         stage.requestFocus();
> 145:         Thread t = new Thread(){

Question: Since you do all the work in a `Platform.runLater`, is this thread needed?

tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 163:

> 161:                 Platform.runLater( () -> {
> 162:                     System.gc();
> 163:                     uncollectedMenuItems.removeIf(ref -> ref.get() == null);

This assumes that the references will all be collected by a single call to `System.gc` with no delay. While it worked on my system, it might be fragile. Have you considered using JMemoryBuddy? Or if that isn't suitable for some reason, maybe consider looping a few times with a sleep in between each call to gc?

tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 165:

> 163:                     uncollectedMenuItems.removeIf(ref -> ref.get() == null);
> 164:                     assertEquals(1, uncollectedMenuItems.size(), "Only the last menuItem should be alive");
> 165:                     memoryLatch.countDown();

If there is a failure, this statement will be unreached. This is why I recommended also doing a countDown on this latch in the `UncaughtExceptionHandler`

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

PR Review: https://git.openjdk.org/jfx/pull/1277#pullrequestreview-1721015874
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1387038813
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1387067229
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1387069616
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1387070763