Make CheckJNI and JNI workarounds mutually exclusive...
...in the opposite direction; needing workarounds turns off CheckJNI.
This isn't necessarily a good idea, but I like the cleanup parts of
this change.
Change-Id: I708235ea3e5cc35ef90b01dd810e097e3ff9dd26
diff --git a/build/Android.common.mk b/build/Android.common.mk
index 20a73b5..51b0f4f 100644
--- a/build/Android.common.mk
+++ b/build/Android.common.mk
@@ -185,6 +185,7 @@
src/runtime.cc \
src/runtime_support.cc \
src/runtime_support_common.cc \
+ src/scoped_thread_list_lock.cc \
src/signal_catcher.cc \
src/space.cc \
src/stack.cc \
diff --git a/src/check_jni.cc b/src/check_jni.cc
index 8efdd1b..b39b521 100644
--- a/src/check_jni.cc
+++ b/src/check_jni.cc
@@ -198,7 +198,7 @@
* obj will be NULL. Otherwise, obj should always be non-NULL
* and valid.
*/
- if (obj != NULL && !Runtime::Current()->GetHeap()->IsHeapAddress(obj)) {
+ if (!Runtime::Current()->GetHeap()->IsHeapAddress(obj)) {
LOG(ERROR) << "JNI ERROR: field operation on invalid " << GetIndirectRefKind(java_object) << ": " << java_object;
JniAbort();
return;
@@ -739,7 +739,7 @@
ScopedJniThreadState ts(env_);
Object* o = Decode<Object*>(ts, java_object);
- if (o != NULL && !Runtime::Current()->GetHeap()->IsHeapAddress(o)) {
+ if (!Runtime::Current()->GetHeap()->IsHeapAddress(o)) {
// TODO: when we remove work_around_app_jni_bugs, this should be impossible.
LOG(ERROR) << "JNI ERROR: native code passing in reference to invalid " << GetIndirectRefKind(java_object) << ": " << java_object;
JniAbort();
diff --git a/src/dalvik_system_VMRuntime.cc b/src/dalvik_system_VMRuntime.cc
index cca5d2b..33e6619 100644
--- a/src/dalvik_system_VMRuntime.cc
+++ b/src/dalvik_system_VMRuntime.cc
@@ -20,8 +20,10 @@
#include "object.h"
#include "object_utils.h"
#include "scoped_heap_lock.h"
+#include "scoped_thread_list_lock.h"
#include "space.h"
#include "thread.h"
+#include "thread_list.h"
#include "JniConstants.h" // Last to avoid problems with LOG redefinition.
#include "toStringArray.h"
@@ -124,18 +126,29 @@
return env->NewStringUTF(Runtime::Current()->GetVersion());
}
+static void DisableCheckJniCallback(Thread* t, void*) {
+ LOG(INFO) << "Disabling CheckJNI for " << *t;
+ t->GetJniEnv()->SetCheckJniEnabled(false);
+}
+
void VMRuntime_setTargetSdkVersion(JNIEnv* env, jobject, jint targetSdkVersion) {
// This is the target SDK version of the app we're about to run.
// Note that targetSdkVersion may be CUR_DEVELOPMENT (10000).
// Note that targetSdkVersion may be 0, meaning "current".
if (targetSdkVersion > 0 && targetSdkVersion <= 13 /* honeycomb-mr2 */) {
- JNIEnvExt* env_ext = reinterpret_cast<JNIEnvExt*>(env);
- // running with CheckJNI forces you to obey the strictest rules.
- if (!env_ext->check_jni) {
- LOG(INFO) << "Turning on JNI app bug workarounds for target SDK version "
- << targetSdkVersion << "...";
- env_ext->vm->work_around_app_jni_bugs = true;
+ Runtime* runtime = Runtime::Current();
+ JavaVMExt* vm = runtime->GetJavaVM();
+
+ if (vm->check_jni) {
+ LOG(WARNING) << "Turning off CheckJNI so we can turn on JNI app bug workarounds...";
+ ScopedThreadListLock thread_list_lock;
+ vm->SetCheckJniEnabled(false);
+ runtime->GetThreadList()->ForEach(DisableCheckJniCallback, NULL);
}
+
+ LOG(INFO) << "Turning on JNI app bug workarounds for target SDK version "
+ << targetSdkVersion << "...";
+ vm->work_around_app_jni_bugs = true;
}
}
diff --git a/src/dalvik_system_VMStack.cc b/src/dalvik_system_VMStack.cc
index a83ad45..8560c22 100644
--- a/src/dalvik_system_VMStack.cc
+++ b/src/dalvik_system_VMStack.cc
@@ -18,6 +18,7 @@
#include "class_loader.h"
#include "object.h"
#include "scoped_heap_lock.h"
+#include "scoped_thread_list_lock.h"
#include "thread_list.h"
#include "JniConstants.h" // Last to avoid problems with LOG redefinition.
diff --git a/src/dalvik_system_Zygote.cc b/src/dalvik_system_Zygote.cc
index 666541f..3f57bfc 100644
--- a/src/dalvik_system_Zygote.cc
+++ b/src/dalvik_system_Zygote.cc
@@ -213,9 +213,9 @@
JavaVMExt* vm = runtime->GetJavaVM();
if (!vm->check_jni) {
LOG(DEBUG) << "Late-enabling -Xcheck:jni";
- vm->EnableCheckJni();
+ vm->SetCheckJniEnabled(true);
// There's only one thread running at this point, so only one JNIEnv to fix up.
- Thread::Current()->GetJniEnv()->EnableCheckJni();
+ Thread::Current()->GetJniEnv()->SetCheckJniEnabled(true);
} else {
LOG(DEBUG) << "Not late-enabling -Xcheck:jni (already on)";
}
diff --git a/src/debugger.cc b/src/debugger.cc
index 0255775..84dd055 100644
--- a/src/debugger.cc
+++ b/src/debugger.cc
@@ -27,6 +27,7 @@
#include "object_utils.h"
#include "ScopedLocalRef.h"
#include "ScopedPrimitiveArray.h"
+#include "scoped_thread_list_lock.h"
#include "space.h"
#include "stack_indirect_reference_table.h"
#include "thread_list.h"
@@ -1624,7 +1625,7 @@
CHECK_EQ(width, sizeof(JDWP::ObjectId));
Object* o = reinterpret_cast<Object*>(f.GetVReg(m, reg));
VLOG(jdwp) << "get array local " << reg << " = " << o;
- if (o != NULL && !Runtime::Current()->GetHeap()->IsHeapAddress(o)) {
+ if (!Runtime::Current()->GetHeap()->IsHeapAddress(o)) {
LOG(FATAL) << "Register " << reg << " expected to hold array: " << o;
}
JDWP::SetObjectId(buf+1, gRegistry->Add(o));
@@ -1640,7 +1641,7 @@
CHECK_EQ(width, sizeof(JDWP::ObjectId));
Object* o = reinterpret_cast<Object*>(f.GetVReg(m, reg));
VLOG(jdwp) << "get object local " << reg << " = " << o;
- if (o != NULL && !Runtime::Current()->GetHeap()->IsHeapAddress(o)) {
+ if (!Runtime::Current()->GetHeap()->IsHeapAddress(o)) {
LOG(FATAL) << "Register " << reg << " expected to hold object: " << o;
}
tag = TagFromObject(o);
diff --git a/src/heap.cc b/src/heap.cc
index 189bb91..13603d9 100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -294,7 +294,10 @@
bool Heap::IsHeapAddress(const Object* obj) {
// Note: we deliberately don't take the lock here, and mustn't test anything that would
// require taking the lock.
- if (obj == NULL || !IsAligned<kObjectAlignment>(obj)) {
+ if (obj == NULL) {
+ return true;
+ }
+ if (!IsAligned<kObjectAlignment>(obj)) {
return false;
}
for (size_t i = 0; i < spaces_.size(); ++i) {
diff --git a/src/java_lang_Thread.cc b/src/java_lang_Thread.cc
index 0338454..f351f20 100644
--- a/src/java_lang_Thread.cc
+++ b/src/java_lang_Thread.cc
@@ -17,6 +17,7 @@
#include "debugger.h"
#include "jni_internal.h"
#include "object.h"
+#include "scoped_thread_list_lock.h"
#include "ScopedUtfChars.h"
#include "thread.h"
#include "thread_list.h"
diff --git a/src/jni_internal.cc b/src/jni_internal.cc
index eb33a5c..64d8203 100644
--- a/src/jni_internal.cc
+++ b/src/jni_internal.cc
@@ -2354,7 +2354,7 @@
}
};
-const JNINativeInterface gNativeInterface = {
+const JNINativeInterface gJniNativeInterface = {
NULL, // reserved0.
NULL, // reserved1.
NULL, // reserved2.
@@ -2598,9 +2598,9 @@
check_jni(false),
critical(false),
monitors("monitors", kMonitorsInitial, kMonitorsMax) {
- functions = unchecked_functions = &gNativeInterface;
+ functions = unchecked_functions = &gJniNativeInterface;
if (vm->check_jni) {
- EnableCheckJni();
+ SetCheckJniEnabled(true);
}
// The JniEnv local reference values must be at a consistent offset or else cross-compilation
// errors will ensue.
@@ -2611,9 +2611,9 @@
JNIEnvExt::~JNIEnvExt() {
}
-void JNIEnvExt::EnableCheckJni() {
- check_jni = true;
- functions = GetCheckJniNativeInterface();
+void JNIEnvExt::SetCheckJniEnabled(bool enabled) {
+ check_jni = enabled;
+ functions = enabled ? GetCheckJniNativeInterface() : &gJniNativeInterface;
}
void JNIEnvExt::DumpReferenceTables() {
@@ -2719,7 +2719,7 @@
}
};
-const JNIInvokeInterface gInvokeInterface = {
+const JNIInvokeInterface gJniInvokeInterface = {
NULL, // reserved0
NULL, // reserved1
NULL, // reserved2
@@ -2745,9 +2745,9 @@
weak_globals(kWeakGlobalsInitial, kWeakGlobalsMax, kWeakGlobal),
libraries_lock("JNI shared libraries map lock"),
libraries(new Libraries) {
- functions = unchecked_functions = &gInvokeInterface;
+ functions = unchecked_functions = &gJniInvokeInterface;
if (options->check_jni_) {
- EnableCheckJni();
+ SetCheckJniEnabled(true);
}
}
@@ -2755,9 +2755,9 @@
delete libraries;
}
-void JavaVMExt::EnableCheckJni() {
- check_jni = true;
- functions = GetCheckJniInvokeInterface();
+void JavaVMExt::SetCheckJniEnabled(bool enabled) {
+ check_jni = enabled;
+ functions = enabled ? GetCheckJniInvokeInterface() : &gJniInvokeInterface;
}
void JavaVMExt::DumpReferenceTables() {
diff --git a/src/jni_internal.h b/src/jni_internal.h
index 79a7af9..021559f 100644
--- a/src/jni_internal.h
+++ b/src/jni_internal.h
@@ -99,7 +99,7 @@
void DumpReferenceTables();
- void EnableCheckJni();
+ void SetCheckJniEnabled(bool enabled);
void VisitRoots(Heap::RootVisitor*, void*);
@@ -143,7 +143,7 @@
void DumpReferenceTables();
- void EnableCheckJni();
+ void SetCheckJniEnabled(bool enabled);
void PushFrame(int capacity);
void PopFrame();
diff --git a/src/monitor.cc b/src/monitor.cc
index 7261bc4..6a7b66f 100644
--- a/src/monitor.cc
+++ b/src/monitor.cc
@@ -28,6 +28,7 @@
#include "mutex.h"
#include "object.h"
#include "object_utils.h"
+#include "scoped_thread_list_lock.h"
#include "stl_util.h"
#include "thread.h"
#include "thread_list.h"
diff --git a/src/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc b/src/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
index c5db4f6..bf356dc 100644
--- a/src/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
+++ b/src/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
@@ -17,6 +17,7 @@
#include "debugger.h"
#include "logging.h"
#include "scoped_heap_lock.h"
+#include "scoped_thread_list_lock.h"
#include "stack.h"
#include "thread_list.h"
diff --git a/src/runtime_support.cc b/src/runtime_support.cc
index 2ecb02e..1828985 100644
--- a/src/runtime_support.cc
+++ b/src/runtime_support.cc
@@ -539,7 +539,7 @@
intptr_t value = *arg_ptr;
Object** value_as_jni_rep = reinterpret_cast<Object**>(value);
Object* value_as_work_around_rep = value_as_jni_rep != NULL ? *value_as_jni_rep : NULL;
- CHECK(Runtime::Current()->GetHeap()->IsHeapAddress(value_as_work_around_rep));
+ CHECK(Runtime::Current()->GetHeap()->IsHeapAddress(value_as_work_around_rep)) << value_as_work_around_rep;
*arg_ptr = reinterpret_cast<intptr_t>(value_as_work_around_rep);
}
diff --git a/src/scoped_thread_list_lock.cc b/src/scoped_thread_list_lock.cc
new file mode 100644
index 0000000..b999c3b
--- /dev/null
+++ b/src/scoped_thread_list_lock.cc
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2012 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 "scoped_thread_list_lock.h"
+
+#include "runtime.h"
+#include "thread_list.h"
+
+namespace art {
+
+ScopedThreadListLock::ScopedThreadListLock() {
+ // Avoid deadlock between two threads trying to SuspendAll
+ // simultaneously by going to kVmWait if the lock cannot be
+ // immediately acquired.
+ ThreadList* thread_list = Runtime::Current()->GetThreadList();
+ if (!thread_list->thread_list_lock_.TryLock()) {
+ Thread* self = Thread::Current();
+ if (self == NULL) {
+ // Self may be null during shutdown, but in that case there's no point going to kVmWait.
+ thread_list->thread_list_lock_.Lock();
+ } else {
+ Thread::State old_thread_state = self->SetState(Thread::kVmWait);
+ thread_list->thread_list_lock_.Lock();
+ // If we have the lock, by definition there's no GC in progress (though we
+ // might be taking the lock in order to start one). We avoid the suspend
+ // check here so we don't risk going to sleep on the thread suspend count lock
+ // while holding the thread list lock.
+ self->SetStateWithoutSuspendCheck(old_thread_state);
+ }
+ }
+}
+
+ScopedThreadListLock::~ScopedThreadListLock() {
+ Runtime::Current()->GetThreadList()->thread_list_lock_.Unlock();
+}
+
+} // namespace art
diff --git a/src/scoped_thread_list_lock.h b/src/scoped_thread_list_lock.h
new file mode 100644
index 0000000..8650c57
--- /dev/null
+++ b/src/scoped_thread_list_lock.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2012 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.
+ */
+
+#ifndef ART_SRC_SCOPED_THREAD_LIST_LOCK_H_
+#define ART_SRC_SCOPED_THREAD_LIST_LOCK_H_
+
+#include "macros.h"
+
+namespace art {
+
+class ScopedThreadListLock {
+ public:
+ ScopedThreadListLock();
+ ~ScopedThreadListLock();
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(ScopedThreadListLock);
+};
+
+} // namespace art
+
+#endif // ART_SRC_SCOPED_THREAD_LIST_LOCK_H_
diff --git a/src/thread_list.cc b/src/thread_list.cc
index b2cf676..9cefc82 100644
--- a/src/thread_list.cc
+++ b/src/thread_list.cc
@@ -20,35 +20,10 @@
#include "debugger.h"
#include "scoped_heap_lock.h"
+#include "scoped_thread_list_lock.h"
namespace art {
-ScopedThreadListLock::ScopedThreadListLock() {
- // Avoid deadlock between two threads trying to SuspendAll
- // simultaneously by going to kVmWait if the lock cannot be
- // immediately acquired.
- ThreadList* thread_list = Runtime::Current()->GetThreadList();
- if (!thread_list->thread_list_lock_.TryLock()) {
- Thread* self = Thread::Current();
- if (self == NULL) {
- // Self may be null during shutdown, but in that case there's no point going to kVmWait.
- thread_list->thread_list_lock_.Lock();
- } else {
- Thread::State old_thread_state = self->SetState(Thread::kVmWait);
- thread_list->thread_list_lock_.Lock();
- // If we have the lock, by definition there's no GC in progress (though we
- // might be taking the lock in order to start one). We avoid the suspend
- // check here so we don't risk going to sleep on the thread suspend count lock
- // while holding the thread list lock.
- self->SetStateWithoutSuspendCheck(old_thread_state);
- }
- }
-}
-
-ScopedThreadListLock::~ScopedThreadListLock() {
- Runtime::Current()->GetThreadList()->thread_list_lock_.Unlock();
-}
-
ThreadList::ThreadList()
: thread_list_lock_("thread list lock", kThreadListLock),
thread_start_cond_("thread_start_cond_"),
diff --git a/src/thread_list.h b/src/thread_list.h
index 4bd9499..07c9514 100644
--- a/src/thread_list.h
+++ b/src/thread_list.h
@@ -87,15 +87,6 @@
DISALLOW_COPY_AND_ASSIGN(ThreadList);
};
-class ScopedThreadListLock {
- public:
- ScopedThreadListLock();
- ~ScopedThreadListLock();
-
- private:
- DISALLOW_COPY_AND_ASSIGN(ScopedThreadListLock);
-};
-
} // namespace art
#endif // ART_SRC_THREAD_LIST_H_
diff --git a/src/trace.cc b/src/trace.cc
index e4be356..6648b85 100644
--- a/src/trace.cc
+++ b/src/trace.cc
@@ -24,6 +24,7 @@
#include "object_utils.h"
#include "os.h"
#include "runtime_support.h"
+#include "scoped_thread_list_lock.h"
#include "thread.h"