On Sat, 4 Nov 2023 14:11:46 GMT, Kevin Rushforth <k...@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
