RFR: 8291736: find_method_handle_intrinsic leaks Method* [v4]

[Coleen Phillimore]

On Thu, 1 Sep 2022 06:42:51 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> The problem with the original code was the ranking and multi-purpose use of the SystemDictionary_lock.  Creating the adapters takes out a lock whose ranking is above SystemDictionary_lock.  Also the tables (and lock for the tables) were shared with code that could call Java code for calling Java to create MethodType, so that code had to release the lock.  Leaking the java.lang.invoke.MethodType is ok because it'll be GC'd.
>> 
>> We've leaked the Method* and adapter for many years now, maybe it's fine to keep leaking it and I should just close this and not try to fix it.
>> 
>> The case of throwing exceptions inside Mutex locked regions is something I thought we have all over the source code.  I couldn't find any explicit calls, but the pattern of doing metaspace allocation inside of a MutexLocker is in enough places that adding an   assert(!thread->owns_locks(), "must release all locks when possibly throwing exceptions"); during in the TRAPS version of Metaspace::allocate() fails immediately.
>> 
>> But it may be that the OOM for Metaspace allocation failure doesn't call into Java for constructing the object and this part is fine.
>
> Right, as per our side-bar discussion, while the general rule is "no throwing exceptions while holding a VM mutex/monitor" it seems that it is safe to only throw OutOfMemoryError because `Universe::gen_out_of_memory_error` doesn't execute any Java code. It either uses one of the pre-allocated OOME instances (if available) and fills in the stacktrace, or else (or in case of secondary OOM) it uses the pre-allocated stackless singleton instance.  This should be documented somewhere of course (and perhaps even programmatically verified somehow `NoJavaCodeVerifier`? :) )
> 
> Also whilst the lower-level code makes it somewhat clear that the only exception that can arise is the metaspace OOME, it is not at all clear just by looking at a callsite like:
> 
> methodHandle m = Method::make_method_handle_intrinsic(iid, signature, CHECK_NULL);
> 
> so perhaps we even need additional macros like CHECK_OOM to verify only OOME can be thrown? (Not in this PR.)

I added a comment above the make_method_handle_intrinsic call about why it's ok to call inside a locked region, and agree that we need better checking for the ability to run Java code from all paths with possible exceptions.  I'm not sure how to do that yet but we should file an RFE.

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

PR: https://git.openjdk.org/jdk/pull/9983