On Mon, 6 Nov 2023 19:22:26 GMT, Johan Vos <[email protected]> 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
