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

[Jose Pereda]

On Thu, 9 Nov 2023 15:17:25 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:
> 
>   Use only 10 cycles instead of 50 (preventing this test to take 50 seconds in case it fails).

Looks good, tested successfully before (fails) and after (passes).
I've left some comments, mostly minor remarks.

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

> 32: import java.lang.ref.WeakReference;
> 33: import java.util.ArrayList;
> 34: 

minor, move `test.util.memory.JMemoryBuddy` here, and remove empty line:


import test.util.Util;
+ import test.util.memory.JMemoryBuddy;
+ import java.lang.ref.WeakReference;
+ import java.util.ArrayList;
import java.util.concurrent.CountDownLatch;

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

> 48: import static org.junit.jupiter.api.Assertions.assertEquals;
> 49: 
> 50: import test.util.memory.JMemoryBuddy;

remove

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

> 131:     }
> 132: 
> 133:     public void createMenuBarWithItemsStage() {

change to `private`

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

> 146:         stage.show();
> 147:         stage.requestFocus();
> 148:         Thread t = new Thread(){

minor, use `new Thread(() -> {...});`(or else add a whitespace before the curly brace: `new Thread() {`)

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

> 157:                         menu.getItems().clear();
> 158:                         MenuItem menuItem = new MenuItem("MyItem");
> 159:                         WeakReference wr = new WeakReference<>(menuItem);

Use type parameter `WeakReference<MenuItem> wr`

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

> 162:                     });
> 163:                 }
> 164:                 Platform.runLater( () -> {

Why is `Platform.runLater` needed here? To wait until the last `Platform.runLater` in the for loop ends? Is it guaranteed that it will be called only after the previous one ends? 

Minor: remove unneeded white space: `Platform.runLater(() -> {`

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

> 164:                 Platform.runLater( () -> {
> 165:                     int strongCount = 0;
> 166:                     for (WeakReference wr: uncollectedMenuItems) {

Use type parameter `WeakReference<MenuItem> wr`

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

> 173:         };
> 174:         t.start();
> 175: 

minor: remove empty line

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

PR Review: https://git.openjdk.org/jfx/pull/1277#pullrequestreview-1723576135
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388631396
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388631765
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388645794
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388635094
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388642837
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388636531
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388647360
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388639830