RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v10]

[Coleen Phillimore]

On Mon, 27 Nov 2023 09:16:30 GMT, Jaroslav Bachorik <jbachorik 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.~
>> 
>> Therefore, we need to perform `jmethodID` lookup for each method in an old class version that is getting purged, and null out the pointer of that `jmethodID` to break the link from `jmethodID` to the method instance that is about to get deallocated.
>> 
>> _(For anyone interested, a much lengthier writeup is available in [my blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_
>
> Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove unnecessary assert

This looks really good.  Thank you for the blog post, it helped understand the problem, which is very convoluted.  I like that the methodID is cleared with purge_previous_version_list.

src/hotspot/share/oops/instanceKlass.cpp line 4236:

> 4234:     if (method != nullptr) {
> 4235:       method->clear_jmethod_id();
> 4236:     }

This loops through the methods in the InstanceKlass that was a previous version klass, and clears the jmethodIDs for all the methods.  Will it clear the jmethodIDs for the EMCP methods also, and should it?
The jmethodID for EMCP methods are replaced with a the new version, so the Method* in this list won't find a matching jmethodID.  Maybe this can be restricted to obsolete methods?

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16662#pullrequestreview-1753984104
PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1408443056