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

[Jaroslav Bachorik]

On Sat, 25 Nov 2023 02:20:18 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This has gotten a lot more complicated. All I was suggesting was if this:
>> 
>> if (with_method_holders) {
>>   method->clear_jmethod_id();
>> }
>> 
>> could be changed to 
>> 
>> if (method->method_holder() == nullptr) {
>>   method->clear_jmethod_id();
>> }
>> 
>> Now I'm not at all sure what you are doing.
>
>> @dholmes-ora Unfortunately, I can not just do `method->method_holder() == nullptr` as `method_holder()` is expanding to `Method::constants()->pool_holder()` and `Method::constants()` is expanding to `Method::constMethod()->constants()`. This can cause SIGSEGV if either `Method::_constMethod` or `ConstMethod::_constants` is NULL.
> 
> I'm getting a strange sense of deja-vu here. This API is flawed if you cannot even ask for the method holder without some intervening value causing a SEGV. I've lost sight of the big picture here in terms of the lifecycle of the Method we are querying, the methodID we may be clearing and the existence or not of a method_holder().

@dholmes-ora I removed the assert. It is not necessary any more as the call to `Method::clear_jmethod_id()` was moved from the more generic `Method::deallocate_contents()` to `InstanceKlass::clear_jmethod_ids()` which is called if and only if the previous class versions are being purged. Because the issue with the method holder is related to `ClassParser` and not fully initialized classes only, the assert can safely be removed. The `Method::clear_jmethod_id()` will never be called in a context when the link to its method holder is broken.

> I've lost sight of the big picture here in terms of the lifecycle of the Method we are querying, the methodID we may be clearing and the existence or not of a method_holder().

I have updated the PR description to correspond to the actual state of affairs - the change is that instead of doing the `jmethodID` pointer maintenance for each `Method::deallocate_contents()` call it will be done only for methods contained by the old class versions that are getting purged. This has two benefits compared to the original proposal:
- we add the overhead of `jmethodID` lookup only to the problematic case of purging old class versions
- method holder is always valid when purging old class versions so we don't need to have checks/asserts for that

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

PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1827409695
PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1827430093