Fix a variety of service protocol bugs.

runtime/vm/service.cc

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/service.h"
 
 #include "include/dart_api.h"
 #include "include/dart_native_api.h"
 #include "platform/globals.h"
 
@@ skipping 1425 matching lines @@
       jsobj.AddProperty("kind", "Free");
       jsobj.AddProperty("valueAsString", "<free>");
       break;
     default:
       UNIMPLEMENTED();
       break;
   }
 }
 
 
-static Breakpoint* LookupBreakpoint(Isolate* isolate, const char* id) {
+static Breakpoint* LookupBreakpoint(Isolate* isolate,
+                                    const char* id,
+                                    ObjectIdRing::LookupResult* result) {
+  *result = ObjectIdRing::kInvalid;
   size_t end_pos = strcspn(id, "/");
   if (end_pos == strlen(id)) {
     return NULL;
   }
   const char* rest = id + end_pos + 1;  // +1 for '/'.
   if (strncmp("breakpoints", id, end_pos) == 0) {
     intptr_t bpt_id = 0;
     Breakpoint* bpt = NULL;
     if (GetIntegerId(rest, &bpt_id)) {
-      bpt = isolate->debugger()->GetBreakpointById(bpt_id);
+      if (bpt_id >= isolate->debugger()->limitBreakpointId()) {
+        *result = ObjectIdRing::kCollected;

[Cutch, 2015/09/11 19:22:19]

We've been asked for a breakpoint id larger than all ids handed out so far, this
should be kInvalid right?

+      } else {
+        bpt = isolate->debugger()->GetBreakpointById(bpt_id);
+        if (bpt) {
+          *result = ObjectIdRing::kValid;
+          return bpt;
+        }
+      }
     }
-    return bpt;
   }
   return NULL;
 }
 
 
 // Scans |isolate|'s message queue looking for a message with |id|.
 // If found, the message is printed to |js| and true is returned.
 // If not found, false is returned.
 static bool PrintMessage(JSONStream* js, Isolate* isolate, const char* id) {
   size_t end_pos = strcspn(id, "/");
@@ skipping 690 matching lines @@
   NULL,
 };
 
 
 static bool RemoveBreakpoint(Isolate* isolate, JSONStream* js) {
   if (!js->HasParam("breakpointId")) {
     PrintMissingParamError(js, "breakpointId");
     return true;
   }
   const char* bpt_id = js->LookupParam("breakpointId");
-  Breakpoint* bpt = LookupBreakpoint(isolate, bpt_id);
+  ObjectIdRing::LookupResult lookup_result;
+  Breakpoint* bpt = LookupBreakpoint(isolate, bpt_id, &lookup_result);
+  // TODO(turnidge): Should we return a different error for bpts whic
+  // have been already removed?
   if (bpt == NULL) {
     PrintInvalidParamError(js, "breakpointId");
     return true;
   }
   isolate->debugger()->RemoveBreakpoint(bpt->id());
   PrintSuccess(js);
   return true;
 }
 
 
@@ skipping 634 matching lines @@
     return true;
   } else if (lookup_result == ObjectIdRing::kCollected) {
     PrintSentinel(js, kCollectedSentinel);
     return true;
   } else if (lookup_result == ObjectIdRing::kExpired) {
     PrintSentinel(js, kExpiredSentinel);
     return true;
   }
 
   // Handle non-heap objects.
-  Breakpoint* bpt = LookupBreakpoint(isolate, id);
+  Breakpoint* bpt = LookupBreakpoint(isolate, id, &lookup_result);
   if (bpt != NULL) {
     bpt->PrintJSON(js);
     return true;
+  } else if (lookup_result == ObjectIdRing::kCollected) {
+    PrintSentinel(js, kCollectedSentinel);
+    return true;
   }
 
   if (PrintMessage(js, isolate, id)) {
     return true;
   }
 
   PrintInvalidParamError(js, "objectId");
   return true;
 }
 
@@ skipping 363 matching lines @@
     ServiceMethodDescriptor& method = service_methods_[i];
     if (strcmp(method_name, method.name) == 0) {
       return &method;
     }
   }
   return NULL;
 }
 
 
 }  // namespace dart