Increase debug information for failed monitor exits
CTS has shown IllegalMonitorStateExceptions where they shouldn't occur.
Ratchet up the debug information in the case of unlocks so the kind of
problem can be better diagnosed. This code is only executed in
exceptional cases and so better debugging wins out over performance.
Also, add helper to dump thread state as something human readable.
Also, JNI internal unit test of monitor enter/exit.
Change-Id: I9c06fbce7e6c9ebbb950a4e400f65c791ebe2ba4
diff --git a/src/jni_internal_test.cc b/src/jni_internal_test.cc
index 62f55b7..97cd969 100644
--- a/src/jni_internal_test.cc
+++ b/src/jni_internal_test.cc
@@ -1460,4 +1460,51 @@
ASSERT_TRUE(env_->GetDirectBufferCapacity(buffer) == sizeof(bytes));
}
+TEST_F(JniInternalTest, MonitorEnterExit) {
+ // Create an object to torture
+ jclass object_class = env_->FindClass("java/lang/Object");
+ ASSERT_TRUE(object_class != NULL);
+ jobject object = env_->AllocObject(object_class);
+ ASSERT_TRUE(object != NULL);
+
+ // Expected class of exceptions
+ jclass imse_class = env_->FindClass("java/lang/IllegalMonitorStateException");
+ ASSERT_TRUE(imse_class != NULL);
+
+ jthrowable thrown_exception;
+
+ // Unlock of unowned monitor
+ env_->MonitorExit(object);
+ EXPECT_TRUE(env_->ExceptionCheck());
+ thrown_exception = env_->ExceptionOccurred();
+ env_->ExceptionClear();
+ EXPECT_TRUE(env_->IsInstanceOf(thrown_exception, imse_class));
+
+ // Lock of unowned monitor
+ env_->MonitorEnter(object);
+ EXPECT_FALSE(env_->ExceptionCheck());
+ // Regular unlock
+ env_->MonitorExit(object);
+ EXPECT_FALSE(env_->ExceptionCheck());
+
+ // Recursively lock a lot
+ size_t max_recursive_lock = 1024;
+ for (size_t i = 0; i < max_recursive_lock; i++) {
+ env_->MonitorEnter(object);
+ EXPECT_FALSE(env_->ExceptionCheck());
+ }
+ // Recursively unlock a lot
+ for (size_t i = 0; i < max_recursive_lock; i++) {
+ env_->MonitorExit(object);
+ EXPECT_FALSE(env_->ExceptionCheck());
+ }
+
+ // Unlock of unowned monitor
+ env_->MonitorExit(object);
+ EXPECT_TRUE(env_->ExceptionCheck());
+ thrown_exception = env_->ExceptionOccurred();
+ env_->ExceptionClear();
+ EXPECT_TRUE(env_->IsInstanceOf(thrown_exception, imse_class));
+}
+
} // namespace art
diff --git a/src/monitor.cc b/src/monitor.cc
index c4deccb..8d6bd190 100644
--- a/src/monitor.cc
+++ b/src/monitor.cc
@@ -229,13 +229,70 @@
}
}
-void ThrowIllegalMonitorStateException(const char* msg) {
- Thread::Current()->ThrowNewException("Ljava/lang/IllegalMonitorStateException;", msg);
+static void ThrowIllegalMonitorStateExceptionF(const char* fmt, ...)
+ __attribute__((format(printf, 1, 2)));
+
+static void ThrowIllegalMonitorStateExceptionF(const char* fmt, ...) {
+ va_list args;
+ va_start(args, fmt);
+ Thread::Current()->ThrowNewExceptionV("Ljava/lang/IllegalMonitorStateException;", fmt, args);
+ va_end(args);
+}
+
+void Monitor::FailedUnlock(Object* obj, Thread* expected_owner, Thread* found_owner,
+ Monitor* mon) {
+ // Acquire thread list lock so threads won't disappear from under us
+ ScopedThreadListLock tll;
+ // Re-read owner now that we hold lock
+ Thread* current_owner = mon != NULL ? mon->owner_ : NULL;
+ std::ostringstream expected_ss;
+ expected_ss << expected_owner;
+ if (current_owner == NULL) {
+ if (found_owner == NULL) {
+ ThrowIllegalMonitorStateExceptionF("unlock of unowned monitor on object of type '%s'"
+ " on thread '%s'",
+ PrettyTypeOf(obj).c_str(), expected_ss.str().c_str());
+ } else {
+ // Race: the original read found an owner but now there is none
+ std::ostringstream found_ss;
+ found_ss << found_owner;
+ ThrowIllegalMonitorStateExceptionF("unlock of monitor owned by '%s' on object of type '%s'"
+ " (where now the monitor appears unowned) on thread '%s'",
+ found_ss.str().c_str(), PrettyTypeOf(obj).c_str(),
+ expected_ss.str().c_str());
+ }
+ } else {
+ std::ostringstream current_ss;
+ current_ss << current_owner;
+ if (found_owner == NULL) {
+ // Race: originally there was no owner, there is now
+ ThrowIllegalMonitorStateExceptionF("unlock of monitor owned by '%s' on object of type '%s'"
+ " (originally believed to be unowned) on thread '%s'",
+ current_ss.str().c_str(), PrettyTypeOf(obj).c_str(),
+ expected_ss.str().c_str());
+ } else {
+ if (found_owner != current_owner) {
+ // Race: originally found and current owner have changed
+ std::ostringstream found_ss;
+ found_ss << found_owner;
+ ThrowIllegalMonitorStateExceptionF("unlock of monitor originally owned by '%s' (now"
+ " owned by '%s') on object of type '%s' on thread '%s'",
+ found_ss.str().c_str(), current_ss.str().c_str(),
+ PrettyTypeOf(obj).c_str(), expected_ss.str().c_str());
+ } else {
+ ThrowIllegalMonitorStateExceptionF("unlock of monitor owned by '%s' on object of type '%s'"
+ " on thread '%s",
+ current_ss.str().c_str(), PrettyTypeOf(obj).c_str(),
+ expected_ss.str().c_str());
+ }
+ }
+ }
}
bool Monitor::Unlock(Thread* self) {
DCHECK(self != NULL);
- if (owner_ == self) {
+ Thread* owner = owner_;
+ if (owner == self) {
// We own the monitor, so nobody else can be in here.
if (lock_count_ == 0) {
owner_ = NULL;
@@ -249,7 +306,7 @@
// We don't own this, so we're not allowed to unlock it.
// The JNI spec says that we should throw IllegalMonitorStateException
// in this case.
- ThrowIllegalMonitorStateException("unlock of unowned monitor");
+ FailedUnlock(obj_, self, owner, this);
return false;
}
return true;
@@ -326,7 +383,7 @@
// Make sure that we hold the lock.
if (owner_ != self) {
- ThrowIllegalMonitorStateException("object not locked by thread before wait()");
+ ThrowIllegalMonitorStateExceptionF("object not locked by thread before wait()");
return;
}
@@ -455,7 +512,7 @@
// Make sure that we hold the lock.
if (owner_ != self) {
- ThrowIllegalMonitorStateException("object not locked by thread before notify()");
+ ThrowIllegalMonitorStateExceptionF("object not locked by thread before notify()");
return;
}
// Signal the first waiting thread in the wait set.
@@ -478,7 +535,7 @@
// Make sure that we hold the lock.
if (owner_ != self) {
- ThrowIllegalMonitorStateException("object not locked by thread before notifyAll()");
+ ThrowIllegalMonitorStateExceptionF("object not locked by thread before notifyAll()");
return;
}
// Signal all threads in the wait set.
@@ -666,7 +723,7 @@
* We do not own the lock. The JVM spec requires that we
* throw an exception in this case.
*/
- ThrowIllegalMonitorStateException("unlock of unowned monitor");
+ FailedUnlock(obj, self, NULL, NULL);
return false;
}
} else {
@@ -694,7 +751,7 @@
if (LW_SHAPE(thin) == LW_SHAPE_THIN) {
// Make sure that 'self' holds the lock.
if (LW_LOCK_OWNER(thin) != self->GetThinLockId()) {
- ThrowIllegalMonitorStateException("object not locked by thread before wait()");
+ ThrowIllegalMonitorStateExceptionF("object not locked by thread before wait()");
return;
}
@@ -717,7 +774,7 @@
if (LW_SHAPE(thin) == LW_SHAPE_THIN) {
// Make sure that 'self' holds the lock.
if (LW_LOCK_OWNER(thin) != self->GetThinLockId()) {
- ThrowIllegalMonitorStateException("object not locked by thread before notify()");
+ ThrowIllegalMonitorStateExceptionF("object not locked by thread before notify()");
return;
}
// no-op; there are no waiters to notify.
@@ -735,7 +792,7 @@
if (LW_SHAPE(thin) == LW_SHAPE_THIN) {
// Make sure that 'self' holds the lock.
if (LW_LOCK_OWNER(thin) != self->GetThinLockId()) {
- ThrowIllegalMonitorStateException("object not locked by thread before notifyAll()");
+ ThrowIllegalMonitorStateExceptionF("object not locked by thread before notifyAll()");
return;
}
// no-op; there are no waiters to notify.
diff --git a/src/monitor.h b/src/monitor.h
index 4d73e37..a79f6ce 100644
--- a/src/monitor.h
+++ b/src/monitor.h
@@ -89,6 +89,8 @@
void LogContentionEvent(Thread* self, uint32_t wait_ms, uint32_t sample_percent, const char* owner_filename, uint32_t owner_line_number);
+ static void FailedUnlock(Object* obj, Thread* expected_owner, Thread* found_owner, Monitor* mon);
+
void Lock(Thread* self);
bool Unlock(Thread* thread);
@@ -105,13 +107,13 @@
static uint32_t lock_profiling_threshold_;
// Which thread currently owns the lock?
- Thread* owner_;
+ Thread* volatile owner_;
// Owner's recursive lock depth.
int lock_count_;
// What object are we part of (for debugging).
- Object* obj_;
+ Object* const obj_;
// Threads currently waiting on this monitor.
Thread* wait_set_;
