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

[Johan Vos]

On Sat, 4 Nov 2023 14:11:46 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Johan Vos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>> 
>>  - Check for null in the Java layer and avoid invoking native methods with a zero pointer.
>>    Zero the relevant pointer in the MenuBarDelegate when a menu is removed.
>>  - Merge remote-tracking branch 'upstream/master' into 8318841-macmenu
>>  - 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.
>
> 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)?

I added the null check on `macMenu`.
I also added the zeroing in the other `remove` method (which indeed is for adding a menu to a menu) and to the `remove` method in `MacMenuBarDelegate`.
Apart from that, I noticed that there were a number of possible scenario's where the Delegate objects would still be used. For example, after removing a menuItem from a menu, one could still invoke methods on that menuItem object. The invocations would propagate to the native layer, causing a crash because the pointer to the `GlassMenu` was already zero (and the native GlassMenu instance was released).

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1383753209