[10] RFR(S): 8191360: Lookup of critical JNI method causes duplicate library loading with leaking handler

[David Holmes]

Looks good!

Thanks,
David

On 5/12/2017 4:27 PM, Tobias Hartmann wrote:
> Hi David,
> 
> thanks for the review!
> 
> On 04.12.2017 22:46, David Holmes wrote:
>> I'm unclear why you added the critical entry to test/jdk/java/lang/ClassLoader/nativeLibrary/libnativeLibraryTest.c ??
>> The impact of that on the test is not clear to me. It also obscures whether the test is fixed just by the actual fix.
> 
> Right, I should have mentioned that in the RFR. The reason is that I first had a fix that only unloaded the shared
> library if no critical method was found, i.e., if critical_entry == NULL. This fixes the crash in the original test
> (because there is no critical version of the method). For additional coverage, I've added a critical entry to the test
> library.
> 
> Thinking about it again, the test changes are not necessary. Here's a webrev without:
> http://cr.openjdk.java.net/~thartmann/8191360/webrev.01/
> 
>> I agree with Vladimir that there is scope for being more efficient with the library handling in this code, but that's
>> future work.
> 
> Right, I'll file a separate RFE and link it to the bug.
> 
> Best regards,
> Tobias
>>> When creating a native wrapper, the compiler checks the shared library for a critical version of the native method
>>> (-XX:+CriticalJNINatives, see comments in JDK-7013347 [1]) that can be called with reduced overhead. This is done in
>>> NativeLookup::lookup_critical_style() with a call to dll_load() to get a handle to the (already loaded) library and a
>>> subsequent call to dll_lookup() to get the entry address of the critical version.
>>>
>>> The problem is that without a call to dll_unload(), this handle to the library will stay live and internal reference
>>> counting will prevent the library from ever being unloaded (even if the native method holder is unloaded). As a result,
>>> static fields in the library code are not reset, causing the NativeLibraryTest to fail due to unexpected field values
>>> once the library is loaded again.
>>>
>>> I've fixed this by closing the handle immediately after retrieving the critical entry address. This is fine because the
>>> library is still kept alive by JNI (see JVM_LoadLibrary). As soon as the holder class and the library are unloaded (see
>>> JVM_UnloadLibrary), the native wrapper that calls 'critical_entry' becomes unreachable and is unloaded as well.
>>>
>>> I've also thought about passing 'RTLD_NOLOAD' to dlopen [2] but although that avoids loading the library, it still
>>> creates a handle (that needs to be closed) if the library is already loaded.
>>>
>>> Tested with failing test and Mach 5 hs-tier1, hs-tier2, hs-precheckin-comp (running).
>>>
>>> Thanks,
>>> Tobias
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-7013347
>>> [2] https://linux.die.net/man/3/dlopen
>>>