RFR: 8268364: jmethod clearing should be done during unloading [v2]

[Coleen Phillimore]

On 7/1/21 9:09 PM, David Holmes wrote:
> On Thu, 1 Jul 2021 15:50:26 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:
>
>>> This patch moves the jmethod clearing to ClassLoaderData::unload() but also adds a check to Method::checked_resolved_jmethod_id() to handle the case where ZGC may be unloading a class but not have gotten to ClassLoaderData::unload() yet.  JVMTI will read a NULL method for checked_resolved_jmethod_id() in this case, and not get a Method that will shortly, or has already been reclaimed in the Metaspace destructor.
>>> Since I was there, I also added Method::is_valid_method() check to checked_resolve_jmethod_id. I don't think it's expensive anymore but it could be added under DEBUG.  Either way method->method_holder()->is_loader_alive() will crash if !is_valid_method so we should leave it.   As I wrote in the related issues, the bogus Method may have been because of a previous set of bugs with post_compiled_method_load events.
>>>
>>> Tested with tiers 1-6 on linux-x64-debug and 1-3 on windows-x64-debug.
>>>
>>> Also ran vmTestbase/nsk/{jdi,jvmti} tests with VM_OPTIONS=-XX:+UseZGC  -XX:ZCollectionInterval=0.01 -XX:ZFragmen
>>> tationLimit=0
>> Coleen Phillimore has updated the pull request incrementally with two additional commits since the last revision:
>>
>>   - ooops fixed typo
>>   - Add a comment about is_loader_alive.
> Hi Coleen,
>
> So IIUC by clearing during unload, rather than in the destructor, it ensures that a jmethodID seen as valid by `Method::checked_resolve_jmethod_id` can't become invalid (due to a loader no longer being alive and being reclaimed) unless there is a safepoint (and assuming the JVM TI agent or application code, is not keeping the loader alive). So this fixes the situation in `jvmti_GetMethodDeclaringClass` as per the bug report, but in general we would have to be careful in the JVM TI implementation to avoid safepoints after validating the jmethodID. Is that correct?

Yes, it is true.  Once you have a Method*, you'd have to have a 
java.lang.Class object for the InstanceKlass on the stack (or a jobject 
to one) if you want to use the Method* later.  This is okay for most 
JVMTI entries because there is already a jobject pointing to the mirror, 
and jvmti_GetMethodDeclaringClass was sort of the exception that didn't 
have the mirror until after the call.
In general if you don't want your Method* to go away, you also need to 
have a methodHandle so that redefinition doesn't clean it up if it's 
redefined, which is a much less frequent but bad situation.
Coleen
>
> Thanks,
> David
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/4643