RFR: 8299423: JavaFX Mac system menubar leaks [v3]

[Florian Kirmaier]

On Thu, 5 Jan 2023 16:15:38 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> I've tested this version with explicit GC, and it still worked. So i guess it's stable.
>> 
>> Ok, so now there are multiple Solutions:
>> 
>> 1. Revert to the previous version, and remove the WeakReference again.
>> 2. Add an explicit reference to the Callable in the MacMenuDelegate. This would make proof reading much easier.
>> 3. Keep it as is.
>> 
>> I guess I would prefer 2. Would that be ok for everyone?
>> 
>> @mstr2 
>> I guess this would have to be done in this method:
>> 
>> 
>> - (void)action:(id)sender
>> {
>>     if (self->jCallback != NULL)
>>     {
>>         GET_MAIN_JENV;
>>         if (env != NULL)
>>         {
>>             (*env)->CallVoidMethod(env, self->jCallback, jMenuActionMethod, NULL);
>>         }
>>     }
>> }
>> 
>> Do you have an example, of how this should look like? I'm neither familiar with C, Objective-C, or this JNI code.
>
>> I've tested this version with explicit GC, and it still worked. So i guess it's stable.
>> 
>> Ok, so now there are multiple Solutions:
>> 
>> 1. Revert to the previous version, and remove the WeakReference again.
>> 2. Add an explicit reference to the Callable in the MacMenuDelegate. This would make proof reading much easier.
>> 3. Keep it as is.
>> 
>> I guess I would prefer 2. Would that be ok for everyone?
>> 
>> @mstr2 I guess this would have to be done in this method:
>> 
>> ```
>> 
>> - (void)action:(id)sender
>> {
>>     if (self->jCallback != NULL)
>>     {
>>         GET_MAIN_JENV;
>>         if (env != NULL)
>>         {
>>             (*env)->CallVoidMethod(env, self->jCallback, jMenuActionMethod, NULL);
>>         }
>>     }
>> }
>> ```
>> 
>> Do you have an example, of how this should look like? I'm neither familiar with C, Objective-C, or this JNI code.
> 
> The `action` method would look something like this:
> ```objective-c
> - (void)action:(id)sender
> {
>     GET_MAIN_JENV;
>     if (env != NULL) {
>         jobject jCallback = (*env)->NewLocalRef(env, self->jCallback);
>         if (jCallback != NULL) {
>             (*env)->CallVoidMethod(env, jCallback, jMenuActionMethod, NULL);
>             (*env)->DeleteLocalRef(env, jCallback);
>         }
>     }
> }
> 
> 
> I've found another use of `jCallback` in `validateMenuItem`.

@mstr2
I've added the suggested nullcheck to the 2 places!

Adding a hard reference to the callback, in the MacMenuDelegate doesn't work without reverting to my original change, which was way more complicated. So I left this part.

As mentioned, I've tested this code with regularly forced "System.gc" and didn't find any issues.

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

PR: https://git.openjdk.org/jfx/pull/987