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

[Kevin Rushforth]

On Thu, 2 Nov 2023 11:39:47 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.

The fix looks good aside from a missing null check on the Java side. I also left a question as to about two possible other place that might need to zero out a pointer.

This will need to be tested in a variety of scenarios, including menus with separators, nested menus, and turning the system menu on and off. What I tested so far looks good.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacMenuDelegate.java line 93:

> 91:         MacMenuDelegate macMenu = (MacMenuDelegate)item;
> 92:         _remove(ptr, macMenu == null ? 0L : macMenu.ptr, pos);
> 93:         macMenu.ptr = 0L;

You need to check for `macMenu == null` here (it can be null in the case of a menu separator, which is why the call to `_remove` on the previous line checks for null).

Question: Does the zeroing out of the pointer also need to be done in the other `remove` method (the one immediately above this one)? I think that method is for the nested menu case, but I'm not sure. Similarly, do you need to zero out the pointer in the `MacMenuBarDelegate::remove` method (called when a menu is removed from the system menu bar)?

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

PR Review: https://git.openjdk.org/jfx/pull/1277#pullrequestreview-1713784373
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1382402575