On Fri, 17 Nov 2023 22:53:58 GMT, Coleen Phillimore <cole...@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
