RFR: 8313816: Accessing jmethodID might lead to spurious crashes

[Jaroslav Bachorik]

On Fri, 17 Nov 2023 22:53:58 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Please, review this fix for a corner case handling of `jmethodID` values.
>> 
>> The issue is related to the interplay between `jmethodID` values and method redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` instance. Once that method gets redefined, the `jmethodID` is updated to point to the last `Method` version. 
>> Unless the method is still on stack/running, in which case the original `jmethodID` will be redirected to the latest `Method` version and at the same time the 'previous' `Method` version will receive a new `jmethodID` pointing to that previous version.
>> 
>> If we happen to capture stacktrace via `GetStackTrace` or `GetAllStackTraces` JVMTI calls while this previous `Method` version is still on stack we will have the corresponding frame identified by a `jmethodID` pointing to that version.
>> However, sooner or later the 'previous' class version becomes eligible for cleanup at what time all contained `Method` instances. The cleanup process will not perform the `jmethodID` pointer maintenance and we will end up with pointers to deallocated memory. 
>> This is caused by the fact that the `jmethodID` lifecycle is bound to `ClassLoaderData` instance and all relevant `jmethodID`s will get batch-updated when the class loader is being released and all its classes are getting unloaded. 
>> 
>> This means that we need to make sure that if a `Method` instance is being deallocate the associated `jmethodID` (if any) must not point to the deallocated instance once we are finished. Unfortunately, we can not just update the `jmethodID` values in bulk when purging an old class version - the per `InstanceKlass` jmethodID cache is present only for the main class version and contains `jmethodID` values for both the old and current method versions. 
>> 
>> Therefore we need to perform `jmethodID` lookup when we are about to deallocate a `Method` instance and clean up the pointer only if that `jmethodID` is pointing to the `Method` instance which is being deallocated.
>> 
>> _(For anyone interested, a much lengthier writeup is available in [my blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_
>
> src/hotspot/share/oops/instanceKlass.cpp line 541:
> 
>> 539:       // The previous version will point to them so they're not totally dangling
>> 540:       assert (!method->on_stack(), "shouldn't be called with methods on stack");
>> 541:       // Do the pointer maintenance before releasing the metadata, but not for incomplete methods
> 
> I'm confused by what you mean by method holder, which I think of as methodHandle.  Or InstanceKlass is the holder of the methods.  Maybe this should be more explicit that it's talking about clearing any associated jmethodIDs.

The method holder is an `InstanceKlass` object which can be retrieved as `method->method_holder()` (I apologize if I am using not completely correct terms - this is what I grokked from the sources). And incomplete methods created by the `ClassParser` from the class data stream will not have the link to that `InstanceKlass` set up if the `ClassParser` is already having its `_klass` field set to a non-null value.

If we are talking about clearing any jmetbodIDs associated with an `InstanceKlass` instance it is not really possible for old method versions because only the current `InstanceKlass` version has the jmethodID cache associated with it and it contains jmethodIDs pointing to bot the old and current methods.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1398009493