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

[Jiangli Zhou]

On Tue, 8 Jun 2021 19:23:35 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>> I added comments to both of these bugs JDK-8268088 and JDK-8268364.
>> 
>> There are places in the JNI spec that don't specify a "valid method ID" and some places just say "a method ID".  We might be able to fix those.  We will crash for invalid method ID in all the JNI entry points.  There is -Xcheck:jni that does check the method ID though.  Guess I'm repeating my comment above.
>> 
>> As far as the new comment goes, for now, it should include JNI until which time we clarify the spec. I don't see the harm in that.
>> 
>> I was trying to think of a way to check jmethodID with SafeFetch or something to to return JVMTI_ERROR_INVALID_METHODID without running into ABA problems.  Maybe that would work and we can deallocate the jmethodID block.
>> 
>> That said, I never saw the PR  66ef04a and I'd want to review that carefully. My first quick click-through was not positive.  I hid JMethodIDBlocks in method.cpp on purpose!  Plus it seems like a huge change for JDK 11.
>
>> See https://docs.oracle.com/en/java/javase/16/docs/specs/jni/design.html
>> 
>> "A field or method ID does not prevent the VM from unloading the class from which the ID has been derived. After the class is unloaded, the method or field ID becomes invalid.
>> ...
>> The programmer must not pass illegal pointers or arguments of the wrong type to JNI functions. Doing so could result in arbitrary consequences, including a corrupted system state or VM crash."
> 
> I had read the first quoted text in "Accessing Fields and Methods" when looking into the spec. It states that the method ID becomes invalid after class unloading, but the VM&JNI implementation has tolerated the invalid ID usages after unloading for long time. The second one that you quoted from "Reporting Programming Errors" has stronger language regarding the usages of illegal pointers. It would be good to clarify the language in  "Accessing Fields and Methods" to not allow the usage of the invalid IDs after class unloading. 
> 
>> From above, we can conclude that both the JNI spec and implementation disallow the use of invalid jmethodIDs. No existing code 3rd party JNI code can run with invalid jmethodIDs without crashing.
> 
> Today's implementation can tolerate the 'invalid' jemthod_ids usages after unloading (by not releasing the memory). 
> 
>> In [JDK-8268088](https://bugs.openjdk.java.net/browse/JDK-8268088), you described a JVMTI-based profiler that "captured stack traces and then symbolized methods using jmethod_ids at a later point during runtime execution, which could happen after unloading the associated classes and loaders."
>> 
>> This is unsafe and can crash with ZGC even if we don't free the JNIMethodBlocks, because `checked_method` could point to a deallocated `Method`. See more in [JDK-8268364](https://bugs.openjdk.java.net/browse/JDK-8268364).
> 
> Agreed, that would become unsafe when we change to release memory for JNIMethodBlock and JNIMethodBlockNodes with unloaded classes. Thanks for filing [JDK-8268364](https://bugs.openjdk.java.net/browse/JDK-8268364).
> 
>> To handle this correctly, the profiler should ensure that the classes with the collected stacks cannot be GCed. This can be done by creating a GlobalRef on the classloader associated with the jmethodID:
>> 
>> * GetMethodDeclaringClass -> GetClassLoader -> NewGlobalRef
>> 
>> The profiler can release the GlobalRef after it has processed the jmethodID.
> 
> I would be very cautious to recommend this practice. I added some comments in [JDK-8268364](https://bugs.openjdk.java.net/browse/JDK-8268364):
> 
> That would keep more memory for longer time. The agent behavior would dictate the memory usages of the VM and prevent classes from being unloaded unnecessarily. If an agent only processes all jmethod_ids at the end, it would keep all loaders alive without being able to be unloaded. Also, a bug (fail to release any of the GoabalRef) in the agent might cause memory leaks.

> There is -Xcheck:jni that does check the method ID though. Guess I'm repeating my comment above.

-Xcheck:jni is not enabled commonly during testing due to other issues. Catching the issue with -Xcheck:jni before production would help (this case does make it more important to enable -Xcheck:jni).

> 
> As far as the new comment goes, for now, it should include JNI until which time we clarify the spec. I don't see the harm in that.
> 

Agreed.

> That said, I never saw the PR [66ef04a](https://github.com/openjdk/jdk/commit/66ef04af110862a1a6b727f12ad5eed9c26cd5a9) and I'd want to review that carefully. My first quick click-through was not positive. 

I opened PR [66ef04a](https://github.com/openjdk/jdk/commit/66ef04af110862a1a6b727f12ad5eed9c26cd5a9) mainly for reference purpose for now. Please don't spend much time reviewing yet :), since we need to address the invalid jmethod_id usages first.
 
> I hid JMethodIDBlocks in method.cpp on purpose! Plus it seems like a huge change for JDK 11.

I moved JNIMethodBlock and JNIMethodBlockNodes class declarations to method.hpp to trigger destructor properly from classLoaderData.cpp in [66ef04a](https://github.com/openjdk/jdk/commit/66ef04af110862a1a6b727f12ad5eed9c26cd5a9). I made the change as an internal fix and intended to only contribute it to JDK head if there was no issue.

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

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