From 598302ac91fd3e990f50e1aa530c3ad61d6d946e Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Wed, 23 Sep 2015 14:52:39 -0700 Subject: [PATCH] Call JNI_OnUnload when class loaders get collected Added test case to 141-class-unload. Bug: 22720414 Change-Id: I0575fae72521520a17587e8b0088bf8112705ad8 --- runtime/gc/heap.cc | 14 ++++ runtime/java_vm_ext.cc | 97 ++++++++++++++++++------- runtime/java_vm_ext.h | 9 ++- test/004-JniTest/expected.txt | 1 + test/004-JniTest/jni_test.cc | 4 +- test/004-ReferenceMap/expected.txt | 1 + test/004-SignalTest/expected.txt | 1 + test/004-StackWalk/expected.txt | 1 + test/004-UnsafeTest/expected.txt | 1 + test/044-proxy/expected.txt | 1 + test/051-thread/expected.txt | 1 + test/088-monitor-verification/expected.txt | 1 + test/115-native-bridge/expected.txt | 1 + test/117-nopatchoat/expected.txt | 3 + test/119-noimage-patchoat/expected.txt | 3 + test/137-cfi/expected.txt | 1 + test/139-register-natives/expected.txt | 1 + test/141-class-unload/expected.txt | 3 + test/141-class-unload/jni_unload.cc | 24 ++++++ test/141-class-unload/src-ex/IntHolder.java | 4 + test/141-class-unload/src/Main.java | 21 ++++++ test/454-get-vreg/expected.txt | 1 + test/455-set-vreg/expected.txt | 1 + test/461-get-reference-vreg/expected.txt | 1 + test/466-get-live-vreg/expected.txt | 1 + test/497-inlining-and-class-loader/expected.txt | 1 + test/Android.libarttest.mk | 1 + 27 files changed, 169 insertions(+), 30 deletions(-) create mode 100644 test/141-class-unload/jni_unload.cc diff --git a/runtime/gc/heap.cc b/runtime/gc/heap.cc index cfe77135b..7d664faa4 100644 --- a/runtime/gc/heap.cc +++ b/runtime/gc/heap.cc @@ -1963,6 +1963,10 @@ HomogeneousSpaceCompactResult Heap::PerformHomogeneousSpaceCompact() { GrowForUtilization(semi_space_collector_); LogGC(kGcCauseHomogeneousSpaceCompact, collector); FinishGC(self, collector::kGcTypeFull); + { + ScopedObjectAccess soa(self); + soa.Vm()->UnloadNativeLibraries(); + } return HomogeneousSpaceCompactResult::kSuccess; } @@ -2104,6 +2108,10 @@ void Heap::TransitionCollector(CollectorType collector_type) { DCHECK(collector != nullptr); LogGC(kGcCauseCollectorTransition, collector); FinishGC(self, collector::kGcTypeFull); + { + ScopedObjectAccess soa(self); + soa.Vm()->UnloadNativeLibraries(); + } int32_t after_allocated = num_bytes_allocated_.LoadSequentiallyConsistent(); int32_t delta_allocated = before_allocated - after_allocated; std::string saved_str; @@ -2588,6 +2596,12 @@ collector::GcType Heap::CollectGarbageInternal(collector::GcType gc_type, FinishGC(self, gc_type); // Inform DDMS that a GC completed. Dbg::GcDidFinish(); + // Unload native libraries for class unloading. We do this after calling FinishGC to prevent + // deadlocks in case the JNI_OnUnload function does allocations. + { + ScopedObjectAccess soa(self); + soa.Vm()->UnloadNativeLibraries(); + } return gc_type; } diff --git a/runtime/java_vm_ext.cc b/runtime/java_vm_ext.cc index 531e03926..ab70d0234 100644 --- a/runtime/java_vm_ext.cc +++ b/runtime/java_vm_ext.cc @@ -60,7 +60,7 @@ class SharedLibrary { : path_(path), handle_(handle), needs_native_bridge_(false), - class_loader_(env->NewGlobalRef(class_loader)), + class_loader_(env->NewWeakGlobalRef(class_loader)), jni_on_load_lock_("JNI_OnLoad lock"), jni_on_load_cond_("JNI_OnLoad condition variable", jni_on_load_lock_), jni_on_load_thread_id_(self->GetThreadId()), @@ -70,11 +70,11 @@ class SharedLibrary { ~SharedLibrary() { Thread* self = Thread::Current(); if (self != nullptr) { - self->GetJniEnv()->DeleteGlobalRef(class_loader_); + self->GetJniEnv()->DeleteWeakGlobalRef(class_loader_); } } - jobject GetClassLoader() const { + jweak GetClassLoader() const { return class_loader_; } @@ -131,7 +131,13 @@ class SharedLibrary { return needs_native_bridge_; } - void* FindSymbol(const std::string& symbol_name) { + void* FindSymbol(const std::string& symbol_name, const char* shorty = nullptr) { + return NeedsNativeBridge() + ? FindSymbolWithNativeBridge(symbol_name.c_str(), shorty) + : FindSymbolWithoutNativeBridge(symbol_name.c_str()); + } + + void* FindSymbolWithoutNativeBridge(const std::string& symbol_name) { CHECK(!NeedsNativeBridge()); return dlsym(handle_, symbol_name.c_str()); @@ -160,9 +166,9 @@ class SharedLibrary { // True if a native bridge is required. bool needs_native_bridge_; - // The ClassLoader this library is associated with, a global JNI reference that is + // The ClassLoader this library is associated with, a weak global JNI reference that is // created/deleted with the scope of the library. - const jobject class_loader_; + const jweak class_loader_; // Guards remaining items. Mutex jni_on_load_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER; @@ -184,7 +190,10 @@ class Libraries { STLDeleteValues(&libraries_); } - void Dump(std::ostream& os) const { + // NO_THREAD_SAFETY_ANALYSIS since this may be called from Dumpable. Dumpable can't be annotated + // properly due to the template. The caller should be holding the jni_libraries_lock_. + void Dump(std::ostream& os) const NO_THREAD_SAFETY_ANALYSIS { + Locks::jni_libraries_lock_->AssertHeld(Thread::Current()); bool first = true; for (const auto& library : libraries_) { if (!first) { @@ -195,16 +204,17 @@ class Libraries { } } - size_t size() const { + size_t size() const REQUIRES(Locks::jni_libraries_lock_) { return libraries_.size(); } - SharedLibrary* Get(const std::string& path) { + SharedLibrary* Get(const std::string& path) REQUIRES(Locks::jni_libraries_lock_) { auto it = libraries_.find(path); return (it == libraries_.end()) ? nullptr : it->second; } - void Put(const std::string& path, SharedLibrary* library) { + void Put(const std::string& path, SharedLibrary* library) + REQUIRES(Locks::jni_libraries_lock_) { libraries_.Put(path, library); } @@ -217,24 +227,18 @@ class Libraries { const mirror::ClassLoader* declaring_class_loader = m->GetDeclaringClass()->GetClassLoader(); ScopedObjectAccessUnchecked soa(Thread::Current()); for (const auto& lib : libraries_) { - SharedLibrary* library = lib.second; + SharedLibrary* const library = lib.second; if (soa.Decode(library->GetClassLoader()) != declaring_class_loader) { // We only search libraries loaded by the appropriate ClassLoader. continue; } // Try the short name then the long name... - void* fn; - if (library->NeedsNativeBridge()) { - const char* shorty = m->GetShorty(); - fn = library->FindSymbolWithNativeBridge(jni_short_name, shorty); - if (fn == nullptr) { - fn = library->FindSymbolWithNativeBridge(jni_long_name, shorty); - } - } else { - fn = library->FindSymbol(jni_short_name); - if (fn == nullptr) { - fn = library->FindSymbol(jni_long_name); - } + const char* shorty = library->NeedsNativeBridge() + ? m->GetShorty() + : nullptr; + void* fn = library->FindSymbol(jni_short_name, shorty); + if (fn == nullptr) { + fn = library->FindSymbol(jni_long_name, shorty); } if (fn != nullptr) { VLOG(jni) << "[Found native code for " << PrettyMethod(m) @@ -249,11 +253,46 @@ class Libraries { return nullptr; } + // Unload native libraries with cleared class loaders. + void UnloadNativeLibraries() + REQUIRES(!Locks::jni_libraries_lock_) + SHARED_REQUIRES(Locks::mutator_lock_) { + ScopedObjectAccessUnchecked soa(Thread::Current()); + typedef void (*JNI_OnUnloadFn)(JavaVM*, void*); + std::vector unload_functions; + { + MutexLock mu(soa.Self(), *Locks::jni_libraries_lock_); + for (auto it = libraries_.begin(); it != libraries_.end(); ) { + SharedLibrary* const library = it->second; + // If class loader is null then it was unloaded, call JNI_OnUnload. + if (soa.Decode(library->GetClassLoader()) == nullptr) { + void* const sym = library->FindSymbol("JNI_OnUnload", nullptr); + if (sym == nullptr) { + VLOG(jni) << "[No JNI_OnUnload found in \"" << library->GetPath() << "\"]"; + } else { + VLOG(jni) << "[JNI_OnUnload found for \"" << library->GetPath() << "\"]"; + JNI_OnUnloadFn jni_on_unload = reinterpret_cast(sym); + unload_functions.push_back(jni_on_unload); + } + delete library; + it = libraries_.erase(it); + } else { + ++it; + } + } + } + // Do this without holding the jni libraries lock to prevent possible deadlocks. + for (JNI_OnUnloadFn fn : unload_functions) { + VLOG(jni) << "Calling JNI_OnUnload"; + (*fn)(soa.Vm(), nullptr); + } + } + private: - AllocationTrackingSafeMap libraries_; + AllocationTrackingSafeMap libraries_ + GUARDED_BY(Locks::jni_libraries_lock_); }; - class JII { public: static jint DestroyJavaVM(JavaVM* vm) { @@ -641,6 +680,10 @@ void JavaVMExt::DumpReferenceTables(std::ostream& os) { } } +void JavaVMExt::UnloadNativeLibraries() { + libraries_.get()->UnloadNativeLibraries(); +} + bool JavaVMExt::LoadNativeLibrary(JNIEnv* env, const std::string& path, jobject class_loader, std::string* error_msg) { error_msg->clear(); @@ -738,10 +781,8 @@ bool JavaVMExt::LoadNativeLibrary(JNIEnv* env, const std::string& path, jobject void* sym; if (needs_native_bridge) { library->SetNeedsNativeBridge(); - sym = library->FindSymbolWithNativeBridge("JNI_OnLoad", nullptr); - } else { - sym = dlsym(handle, "JNI_OnLoad"); } + sym = library->FindSymbol("JNI_OnLoad", nullptr); if (sym == nullptr) { VLOG(jni) << "[No JNI_OnLoad found in \"" << path << "\"]"; was_successful = true; diff --git a/runtime/java_vm_ext.h b/runtime/java_vm_ext.h index b539bbdba..c1fbdc038 100644 --- a/runtime/java_vm_ext.h +++ b/runtime/java_vm_ext.h @@ -88,6 +88,11 @@ class JavaVMExt : public JavaVM { bool LoadNativeLibrary(JNIEnv* env, const std::string& path, jobject javaLoader, std::string* error_msg); + // Unload native libraries with cleared class loaders. + void UnloadNativeLibraries() + REQUIRES(!Locks::jni_libraries_lock_) + SHARED_REQUIRES(Locks::mutator_lock_); + /** * Returns a pointer to the code for the native method 'm', found * using dlsym(3) on every native library that's been loaded so far. @@ -184,7 +189,9 @@ class JavaVMExt : public JavaVM { // Not guarded by globals_lock since we sometimes use SynchronizedGet in Thread::DecodeJObject. IndirectReferenceTable globals_; - std::unique_ptr libraries_ GUARDED_BY(Locks::jni_libraries_lock_); + // No lock annotation since UnloadNativeLibraries is called on libraries_ but locks the + // jni_libraries_lock_ internally. + std::unique_ptr libraries_; // Used by -Xcheck:jni. const JNIInvokeInterface* const unchecked_functions_; diff --git a/test/004-JniTest/expected.txt b/test/004-JniTest/expected.txt index 49d9cc0d5..86ab37e1e 100644 --- a/test/004-JniTest/expected.txt +++ b/test/004-JniTest/expected.txt @@ -1,3 +1,4 @@ +JNI_OnLoad called Super. Super. Subclass. diff --git a/test/004-JniTest/jni_test.cc b/test/004-JniTest/jni_test.cc index db0dd3277..370bd6ad2 100644 --- a/test/004-JniTest/jni_test.cc +++ b/test/004-JniTest/jni_test.cc @@ -15,8 +15,9 @@ */ #include -#include +#include #include +#include #include #include "jni.h" @@ -31,6 +32,7 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM *vm, void *) { assert(vm != nullptr); assert(jvm == nullptr); jvm = vm; + std::cout << "JNI_OnLoad called" << std::endl; return JNI_VERSION_1_6; } diff --git a/test/004-ReferenceMap/expected.txt b/test/004-ReferenceMap/expected.txt index e69de29bb..6a5618ebc 100644 --- a/test/004-ReferenceMap/expected.txt +++ b/test/004-ReferenceMap/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/004-SignalTest/expected.txt b/test/004-SignalTest/expected.txt index fd5ec0006..b3a0e1cbe 100644 --- a/test/004-SignalTest/expected.txt +++ b/test/004-SignalTest/expected.txt @@ -1,3 +1,4 @@ +JNI_OnLoad called init signal test Caught NullPointerException Caught StackOverflowError diff --git a/test/004-StackWalk/expected.txt b/test/004-StackWalk/expected.txt index bde00246a..5af68cd85 100644 --- a/test/004-StackWalk/expected.txt +++ b/test/004-StackWalk/expected.txt @@ -1,3 +1,4 @@ +JNI_OnLoad called 1st call 172001234567891011121314151617181920652310201919 2nd call diff --git a/test/004-UnsafeTest/expected.txt b/test/004-UnsafeTest/expected.txt index e69de29bb..6a5618ebc 100644 --- a/test/004-UnsafeTest/expected.txt +++ b/test/004-UnsafeTest/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/044-proxy/expected.txt b/test/044-proxy/expected.txt index f86948ad6..052c8faf1 100644 --- a/test/044-proxy/expected.txt +++ b/test/044-proxy/expected.txt @@ -93,4 +93,5 @@ Invocation of public abstract java.lang.String NarrowingTest$I2.foo() Got expected exception Proxy narrowed invocation return type passed 5.8 +JNI_OnLoad called callback diff --git a/test/051-thread/expected.txt b/test/051-thread/expected.txt index 54e34af3a..c6cd4f8be 100644 --- a/test/051-thread/expected.txt +++ b/test/051-thread/expected.txt @@ -1,3 +1,4 @@ +JNI_OnLoad called thread test starting testThreadCapacity thread count: 512 testThreadDaemons starting thread 'TestDaemonThread' diff --git a/test/088-monitor-verification/expected.txt b/test/088-monitor-verification/expected.txt index 13b8c7397..f252f6f2e 100644 --- a/test/088-monitor-verification/expected.txt +++ b/test/088-monitor-verification/expected.txt @@ -1,3 +1,4 @@ +JNI_OnLoad called recursiveSync ok nestedMayThrow ok constantLock ok diff --git a/test/115-native-bridge/expected.txt b/test/115-native-bridge/expected.txt index 372ecd048..b003307ab 100644 --- a/test/115-native-bridge/expected.txt +++ b/test/115-native-bridge/expected.txt @@ -17,6 +17,7 @@ Test ART callbacks: all JNI function number is 11. name:testSignal, signature:()I, shorty:I. name:testZeroLengthByteBuffers, signature:()V, shorty:V. trampoline_JNI_OnLoad called! +JNI_OnLoad called Getting trampoline for Java_Main_testFindClassOnAttachedNativeThread with shorty V. trampoline_Java_Main_testFindClassOnAttachedNativeThread called! Getting trampoline for Java_Main_testFindFieldOnAttachedNativeThreadNative with shorty V. diff --git a/test/117-nopatchoat/expected.txt b/test/117-nopatchoat/expected.txt index 5cc02d166..0cd4715d0 100644 --- a/test/117-nopatchoat/expected.txt +++ b/test/117-nopatchoat/expected.txt @@ -1,9 +1,12 @@ Run without dex2oat/patchoat +JNI_OnLoad called dex2oat & patchoat are disabled, has oat is true, has executable oat is expected. This is a function call Run with dexoat/patchoat +JNI_OnLoad called dex2oat & patchoat are enabled, has oat is true, has executable oat is expected. This is a function call Run default +JNI_OnLoad called dex2oat & patchoat are enabled, has oat is true, has executable oat is expected. This is a function call diff --git a/test/119-noimage-patchoat/expected.txt b/test/119-noimage-patchoat/expected.txt index ed136621c..9b9db58fc 100644 --- a/test/119-noimage-patchoat/expected.txt +++ b/test/119-noimage-patchoat/expected.txt @@ -1,8 +1,11 @@ Run -Xnoimage-dex2oat -Xpatchoat:/system/bin/false +JNI_OnLoad called Has image is false, is image dex2oat enabled is false. Run -Xnoimage-dex2oat -Xpatchoat:/system/bin/false -Xno-dex-file-fallback Failed to initialize runtime (check log for details) Run -Ximage-dex2oat +JNI_OnLoad called Has image is true, is image dex2oat enabled is true. Run default +JNI_OnLoad called Has image is true, is image dex2oat enabled is true. diff --git a/test/137-cfi/expected.txt b/test/137-cfi/expected.txt index e69de29bb..6a5618ebc 100644 --- a/test/137-cfi/expected.txt +++ b/test/137-cfi/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/139-register-natives/expected.txt b/test/139-register-natives/expected.txt index e69de29bb..6a5618ebc 100644 --- a/test/139-register-natives/expected.txt +++ b/test/139-register-natives/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/141-class-unload/expected.txt b/test/141-class-unload/expected.txt index be2671eff..124398f19 100644 --- a/test/141-class-unload/expected.txt +++ b/test/141-class-unload/expected.txt @@ -7,3 +7,6 @@ null null loader null false loader null false +JNI_OnLoad called +JNI_OnUnload called +null diff --git a/test/141-class-unload/jni_unload.cc b/test/141-class-unload/jni_unload.cc new file mode 100644 index 000000000..894f28c7a --- /dev/null +++ b/test/141-class-unload/jni_unload.cc @@ -0,0 +1,24 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "jni.h" + +#include + +extern "C" JNIEXPORT void JNI_OnUnload(JavaVM*, void *) { + // std::cout since LOG(INFO) adds extra stuff like pid. + std::cout << "JNI_OnUnload called" << std::endl; +} diff --git a/test/141-class-unload/src-ex/IntHolder.java b/test/141-class-unload/src-ex/IntHolder.java index 0a1c1e6f7..b4651af91 100644 --- a/test/141-class-unload/src-ex/IntHolder.java +++ b/test/141-class-unload/src-ex/IntHolder.java @@ -30,4 +30,8 @@ public class IntHolder { public static void runGC() { Runtime.getRuntime().gc(); } + + public static void loadLibrary(String name) { + System.loadLibrary(name); + } } diff --git a/test/141-class-unload/src/Main.java b/test/141-class-unload/src/Main.java index 3a2ac9be3..da15746d1 100644 --- a/test/141-class-unload/src/Main.java +++ b/test/141-class-unload/src/Main.java @@ -20,8 +20,10 @@ import java.lang.reflect.Method; public class Main { static final String DEX_FILE = System.getenv("DEX_LOCATION") + "/141-class-unload-ex.jar"; + static String nativeLibraryName; public static void main(String[] args) throws Exception { + nativeLibraryName = args[0]; Class pathClassLoader = Class.forName("dalvik.system.PathClassLoader"); if (pathClassLoader == null) { throw new AssertionError("Couldn't find path class loader class"); @@ -34,6 +36,8 @@ public class Main { testNoUnloadInvoke(constructor); // Test that we don't unload if we have an instance. testNoUnloadInstance(constructor); + // Test JNI_OnLoad and JNI_OnUnload. + testLoadAndUnloadLibrary(constructor); // Stress test to make sure we dont leak memory. stressTest(constructor); } catch (Exception e) { @@ -63,6 +67,14 @@ public class Main { System.out.println(loader.get()); } + private static void testLoadAndUnloadLibrary(Constructor constructor) throws Exception { + WeakReference loader = setUpLoadLibrary(constructor); + // No strong refernces to class loader, should get unloaded. + Runtime.getRuntime().gc(); + // If the weak reference is cleared, then it was unloaded. + System.out.println(loader.get()); + } + private static void testNoUnloadInvoke(Constructor constructor) throws Exception { WeakReference loader = new WeakReference((ClassLoader) constructor.newInstance( @@ -109,4 +121,13 @@ public class Main { return new WeakReference(loader); } + private static WeakReference setUpLoadLibrary(Constructor constructor) + throws Exception { + ClassLoader loader = (ClassLoader) constructor.newInstance( + DEX_FILE, ClassLoader.getSystemClassLoader()); + Class intHolder = loader.loadClass("IntHolder"); + Method setValue = intHolder.getDeclaredMethod("loadLibrary", String.class); + setValue.invoke(intHolder, nativeLibraryName); + return new WeakReference(loader); + } } diff --git a/test/454-get-vreg/expected.txt b/test/454-get-vreg/expected.txt index e69de29bb..6a5618ebc 100644 --- a/test/454-get-vreg/expected.txt +++ b/test/454-get-vreg/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/455-set-vreg/expected.txt b/test/455-set-vreg/expected.txt index e69de29bb..6a5618ebc 100644 --- a/test/455-set-vreg/expected.txt +++ b/test/455-set-vreg/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/461-get-reference-vreg/expected.txt b/test/461-get-reference-vreg/expected.txt index e69de29bb..6a5618ebc 100644 --- a/test/461-get-reference-vreg/expected.txt +++ b/test/461-get-reference-vreg/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/466-get-live-vreg/expected.txt b/test/466-get-live-vreg/expected.txt index e69de29bb..6a5618ebc 100644 --- a/test/466-get-live-vreg/expected.txt +++ b/test/466-get-live-vreg/expected.txt @@ -0,0 +1 @@ +JNI_OnLoad called diff --git a/test/497-inlining-and-class-loader/expected.txt b/test/497-inlining-and-class-loader/expected.txt index f5b9fe07d..905dbfd2c 100644 --- a/test/497-inlining-and-class-loader/expected.txt +++ b/test/497-inlining-and-class-loader/expected.txt @@ -1,3 +1,4 @@ +JNI_OnLoad called java.lang.Exception at Main.$noinline$bar(Main.java:124) at Level2.$inline$bar(Level1.java:25) diff --git a/test/Android.libarttest.mk b/test/Android.libarttest.mk index 7f05a043d..e43ea90ba 100644 --- a/test/Android.libarttest.mk +++ b/test/Android.libarttest.mk @@ -33,6 +33,7 @@ LIBARTTEST_COMMON_SRC_FILES := \ 1337-gc-coverage/gc_coverage.cc \ 137-cfi/cfi.cc \ 139-register-natives/regnative.cc \ + 141-class-unload/jni_unload.cc \ 454-get-vreg/get_vreg_jni.cc \ 455-set-vreg/set_vreg_jni.cc \ 457-regs/regs_jni.cc \ -- 2.11.0