RFR: JDK-8268088: Clarify Method::clear_jmethod_ids() related comments in ClassLoaderData::~ClassLoaderData() [v2]

[Jiangli Zhou]

On Tue, 8 Jun 2021 00:53:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>>> This comment looks like an expanded version of the one I wrote a long time ago and correct. 
>> 
>> Thanks for confirming the correctness!
>> 
>>> We were wondering if this is a feature that we actually need.
>> 
>> That seems to be the case. I learned this the hard way with crashes observed in production when attempting to release memory for stale jmethod_ids from VM side. Clarifying the comments hopefully will benefit others and avoid similar issues in the future.
>
> If the issue is specific to JVMTI code only, the comment should reflect that. Currently it seems to suggest that all possible JNI code could use an invalid jmethodID. However, using an invalid jmethodID with JNI APIs such as CallStaticVoidMethod will lead to a crash. I don't think the JNI spec allows invalid jmethodID to be used.
> 
> Furthermore, we should consider freeing the JNIMethodBlock when JVMTI is not enabled, so we can avoid the memory leak.
> 
> We may even want to update the JVMTI spec to disallow invalid jmethodID, but will that introduce incompatibility with 3rd party code?
> 
> Lastly, do we have any existing test cases that cover the use of invalid jmethodIDs?

Hi Ioi, thanks for the thoughts!

> If the issue is specific to JVMTI code only, the comment should reflect that. Currently it seems to suggest that all possible JNI code could use an invalid jmethodID. However, using an invalid jmethodID with JNI APIs such as CallStaticVoidMethod will lead to a crash.

The issue that we encountered recently was with a JVMTI profiling agent. However, we can't conclude from that it is only restricted to JVMTI usages. Since we don't have any evidences to exclude non-JVMTI cases, narrowing down the scope of Coleen's existing comment is not a good idea. Expending the comment with clarifications and example (JVMTI agent) is appropriate at this point.

> I don't think the JNI spec allows invalid jmethodID to be used.

I haven't found the specific language in the JNI spec. The quoted spec language in updated comment does not strongly indicate that.

> 
> Furthermore, we should consider freeing the JNIMethodBlock when JVMTI is not enabled, so we can avoid the memory leak.
> 

It's not just JVMTI vs non-JVMTI issues. The JNIMethodBlocks are not always created. When application native code creates (directly or indirectly) jmethod_ids, how does VM know when it's safe to release the memory? It faces the same issue as the JVMTI agent case. https://github.com/openjdk/jdk/commit/66ef04af110862a1a6b727f12ad5eed9c26cd5a9 addresses from unloading point view. However, that's unsafe either.

> We may even want to update the JVMTI spec to disallow invalid jmethodID, but will that introduce incompatibility with 3rd party code?
> 

If we update the JNI spec to ban such usages and fix all existing third party code (if we can), we can change the VM to release memory safely. That would be welcomed by developers. However, any unfixed native code would still run into risks of crashes.

> Lastly, do we have any existing test cases that cover the use of invalid jmethodIDs?

Unfortunately there's no existing test case. It would help prevent the crash in production if we had a test case that caught the issue early. It's a good to-do item in addition to the comment clarification, before looking into a potential longer term solution for the memory leak.

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

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