Merge "Manage JDWP errors related to garbage collection."

2 files changed, 22 insertions, 9 deletions

diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 8e67228..bcf7267 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -796,18 +796,37 @@ JDWP::JdwpError Dbg::GetReferringObjects(JDWP::ObjectId object_id, int32_t max_c
 
 JDWP::JdwpError Dbg::DisableCollection(JDWP::ObjectId object_id)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+  mirror::Object* o = gRegistry->Get<mirror::Object*>(object_id);
+  if (o == NULL || o == ObjectRegistry::kInvalidObject) {
+    return JDWP::ERR_INVALID_OBJECT;
+  }
   gRegistry->DisableCollection(object_id);
   return JDWP::ERR_NONE;
 }
 
 JDWP::JdwpError Dbg::EnableCollection(JDWP::ObjectId object_id)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+  mirror::Object* o = gRegistry->Get<mirror::Object*>(object_id);
+  // Unlike DisableCollection, JDWP specs do not state an invalid object causes an error. The RI
+  // also ignores these cases and never return an error. However it's not obvious why this command
+  // should behave differently from DisableCollection and IsCollected commands. So let's be more
+  // strict and return an error if this happens.
+  if (o == NULL || o == ObjectRegistry::kInvalidObject) {
+    return JDWP::ERR_INVALID_OBJECT;
+  }
   gRegistry->EnableCollection(object_id);
   return JDWP::ERR_NONE;
 }
 
 JDWP::JdwpError Dbg::IsCollected(JDWP::ObjectId object_id, bool& is_collected)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+  mirror::Object* o = gRegistry->Get<mirror::Object*>(object_id);
+  // JDWP specs state an INVALID_OBJECT error is returned if the object ID is not valid. However
+  // the RI seems to ignore this and does not return any error in this case. Let's comply with
+  // JDWP specs here.
+  if (o == NULL || o == ObjectRegistry::kInvalidObject) {
+    return JDWP::ERR_INVALID_OBJECT;
+  }
   is_collected = gRegistry->IsCollected(object_id);
   return JDWP::ERR_NONE;
 }
diff --git a/runtime/jdwp/object_registry.cc b/runtime/jdwp/object_registry.cc
index 7408e92..369eddd 100644
--- a/runtime/jdwp/object_registry.cc
+++ b/runtime/jdwp/object_registry.cc
@@ -133,9 +133,7 @@ void ObjectRegistry::DisableCollection(JDWP::ObjectId id) {
   Thread* self = Thread::Current();
   MutexLock mu(self, lock_);
   id_iterator it = id_to_entry_.find(id);
-  if (it == id_to_entry_.end()) {
-    return;
-  }
+  CHECK(it != id_to_entry_.end());
   Promote(*(it->second));
 }
 
@@ -143,9 +141,7 @@ void ObjectRegistry::EnableCollection(JDWP::ObjectId id) {
   Thread* self = Thread::Current();
   MutexLock mu(self, lock_);
   id_iterator it = id_to_entry_.find(id);
-  if (it == id_to_entry_.end()) {
-    return;
-  }
+  CHECK(it != id_to_entry_.end());
   Demote(*(it->second));
 }
 
@@ -175,9 +171,7 @@ bool ObjectRegistry::IsCollected(JDWP::ObjectId id) {
   Thread* self = Thread::Current();
   MutexLock mu(self, lock_);
   id_iterator it = id_to_entry_.find(id);
-  if (it == id_to_entry_.end()) {
-    return true;  // TODO: can we report that this was an invalid id?
-  }
+  CHECK(it != id_to_entry_.end());
 
   ObjectRegistryEntry& entry = *(it->second);
   if (entry.jni_reference_type == JNIWeakGlobalRefType) {