RFR: 8258836: JNI local refs exceed capacity getDiagnosticCommandInfo [v2]

[Severin Gehwolf]

On Tue, 19 Jan 2021 19:55:22 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Severin Gehwolf 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 four additional commits since the last revision:
>> 
>>  - Merge test files into one
>>  - Adress review feedback from dholmes
>>  - Merge branch 'master' into JDK-8258836-check-jni-mbeanserver
>>  - 8258836: JNI local refs exceed capacity getDiagnosticCommandInfo
>
> src/jdk.management/share/native/libmanagement_ext/DiagnosticCommandImpl.c line 113:
> 
>> 111:       return NULL;
>> 112:     }
>> 113:     (*env)->PopLocalFrame(env, obj);
> 
> This needs to assign the result to `obj` in order to get the affect you are looking for. Otherwise it is really no different than passing in `NULL.`, and in the `SetObjectArrayElement` that follows you'll end up assigning a popped localref. I'm surprised this didn't result in some sort of test failure. Are you testing with `num_args > 1`? Perhaps the localref frames are not eagerly reused.

Doh! Thanks Chris! Fixed. It did result in test failures (for fastdebug build). For example https://github.com/jerboaa/jdk/runs/1727117374?check_suite_focus=true Sorry for missing this.

> src/jdk.management/share/native/libmanagement_ext/DiagnosticCommandImpl.c line 172:
> 
>> 170:   }
>> 171:   if (num_commands == 0) {
>> 172:       (*env)->PopLocalFrame(env, result);
> 
> Also here you need to assign the result to `result`.

Fixed.

> src/jdk.management/share/native/libmanagement_ext/DiagnosticCommandImpl.c line 222:
> 
>> 220:           return NULL;
>> 221:       }
>> 222:       (*env)->PopLocalFrame(env, obj);
> 
> ...and assign to `obj` here.

Fixed.

> src/jdk.management/share/native/libmanagement_ext/DiagnosticCommandImpl.c line 227:
> 
>> 225:       EXCEPTION_CHECK_AND_FREE(dcmd_info_array);
>> 226:   }
>> 227:   (*env)->PopLocalFrame(env, result);
> 
> ...and assign to `result` here.

Fixed now.

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

PR: https://git.openjdk.java.net/jdk/pull/2130