[vm_service] Deprecate Stack.messages

Make VM always return empty array in the response.

Current implementation for this field comes with a bunch of complexity
because it locks message handler and then invokes Dart code which
makes it difficult to reason about various invariants. This code is 
furthermore demonstrated to cause deadlocks. Given that nobody uses 
it - it is simpler to remove this code altogether.

Fixes https://github.com/flutter/flutter/issues/185156

TEST=ci

Change-Id: I497210e0f1542860caa0d765d634f8ec6a6a6964
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/496340
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Slava Egorov <vegorov@google.com>

pkg/vm_service/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 15.2.0
+- Update to version `4.22` of the spec.
+- Deprecate `Message` type and `messages` field of `Stack` type.
+
 ## 15.1.0
 - Update to version `4.21` of the spec.
 - Add `Pointer` to `InstanceKind`.

pkg/vm_service/java/version.properties

@@ -1 +1 @@
-version=4.21
+version=4.22

pkg/vm_service/pubspec.yaml

@@ -1,5 +1,5 @@
 name: vm_service
-version: 15.1.0
+version: 15.2.0
 description: >-
   A library to communicate with a service implementing the Dart VM
   service protocol.

pkg/vm_service/test/get_stack_rpc_test.dart

@@ -4,7 +4,6 @@
 
 import 'dart:async';
 import 'dart:developer';
-import 'dart:isolate';
 
 import 'package:test/test.dart';
 import 'package:vm_service/vm_service.dart';
@@ -12,16 +11,14 @@ import 'package:vm_service/vm_service.dart';
 import 'common/service_test_common.dart';
 import 'common/test_helper.dart';
 
-const LINE_A = 25;
+const LINE_A = 22;
 
 int counter = 0;
-final port = RawReceivePort(msgHandler);
 
 // This name is used in a test below.
 void msgHandler(_) {}
 
 void periodicTask(_) {
-  port.sendPort.send(34);
   debugger(message: 'fo', when: true);
   counter++;
   if (counter % 300 == 0) {
@@ -60,39 +57,12 @@ final tests = <IsolateTest>[
       expect(frame.function, isNotNull);
       expect(frame.location, isNotNull);
     }
-
-    // Sanity check.
-    final messages = stack.messages!;
-    expect(messages.length, greaterThanOrEqualTo(1));
-
-    // Iterate over messages.
-    int messageDepth = 0;
-    // objectId of message to be handled by msgHandler.
-    String? msgHandlerObjectId;
-    for (final message in messages) {
-      print('checking message $messageDepth');
-      expect(message.index, messageDepth++);
-      expect(message.size, greaterThanOrEqualTo(0));
-      expect(message.handler, isNotNull);
-      expect(message.location, isNotNull);
-      if (message.handler!.name!.contains('msgHandler')) {
-        msgHandlerObjectId = message.messageObjectId;
-      }
-    }
-    expect(msgHandlerObjectId, isNotNull);
-
-    // Get object.
-    final object = await service.getObject(
-      isolateId,
-      msgHandlerObjectId!,
-    ) as Instance;
-    expect(object.valueAsString, '34');
-  }
+  },
 ];
 
 void main([args = const <String>[]]) => runIsolateTests(
-      args,
-      tests,
-      'get_stack_rpc_test.dart',
-      testeeBefore: startTimer,
-    );
+  args,
+  tests,
+  'get_stack_rpc_test.dart',
+  testeeBefore: startTimer,
+);

pkg/vm_service_interface/lib/src/vm_service_interface.dart

@@ -17,7 +17,7 @@ import 'service_extension_registry.dart';
 
 export 'service_extension_registry.dart' show ServiceExtensionRegistry;
 
-const String vmServiceVersion = '4.21.0';
+const String vmServiceVersion = '4.22.0';
 
 /// A class representation of the Dart VM Service Protocol.
 abstract interface class VmServiceInterface {
@@ -28,8 +28,11 @@ abstract interface class VmServiceInterface {
   Stream<Event> onEvent(String streamId);
 
   /// Handler for calling extra service extensions.
-  Future<Response> callServiceExtension(String method,
-      {String? isolateId, Map<String, dynamic>? args});
+  Future<Response> callServiceExtension(
+    String method, {
+    String? isolateId,
+    Map<String, dynamic>? args,
+  });
 
   /// Invoked by the Dart Development Service (DDS) immediately after it
   /// connects.
@@ -360,8 +363,11 @@ abstract interface class VmServiceInterface {
   ///
   /// This method will throw a [SentinelException] in the case a [Sentinel] is
   /// returned.
-  Future<AllocationProfile> getAllocationProfile(String isolateId,
-      {bool? reset, bool? gc});
+  Future<AllocationProfile> getAllocationProfile(
+    String isolateId, {
+    bool? reset,
+    bool? gc,
+  });
 
   /// The `getAllocationTraces` RPC allows for the retrieval of allocation
   /// traces for objects of a specific set of types (see
@@ -421,7 +427,10 @@ abstract interface class VmServiceInterface {
   /// This method will throw a [SentinelException] in the case a [Sentinel] is
   /// returned.
   Future<CpuSamples> getCpuSamples(
-      String isolateId, int timeOriginMicros, int timeExtentMicros);
+    String isolateId,
+    int timeOriginMicros,
+    int timeExtentMicros,
+  );
 
   /// The `getFlagList` RPC returns a list of all command line flags in the VM
   /// along with their current values.
@@ -697,8 +706,11 @@ abstract interface class VmServiceInterface {
   ///
   /// This method will throw a [SentinelException] in the case a [Sentinel] is
   /// returned.
-  Future<PerfettoCpuSamples> getPerfettoCpuSamples(String isolateId,
-      {int? timeOriginMicros, int? timeExtentMicros});
+  Future<PerfettoCpuSamples> getPerfettoCpuSamples(
+    String isolateId, {
+    int? timeOriginMicros,
+    int? timeExtentMicros,
+  });
 
   /// The `getPerfettoVMTimeline` RPC is used to retrieve an object which
   /// contains a VM timeline trace represented in Perfetto's proto format. See
@@ -731,8 +743,10 @@ abstract interface class VmServiceInterface {
   /// or Perfettofile, an [RPCError] with error code `114`, `invalid timeline
   /// request`, will be returned as timeline events are written directly to a
   /// file, and thus cannot be retrieved through the VM Service, in these modes.
-  Future<PerfettoTimeline> getPerfettoVMTimeline(
-      {int? timeOriginMicros, int? timeExtentMicros});
+  Future<PerfettoTimeline> getPerfettoVMTimeline({
+    int? timeOriginMicros,
+    int? timeExtentMicros,
+  });
 
   /// The `getPorts` RPC is used to retrieve the list of `ReceivePort` instances
   /// for a given isolate.
@@ -952,8 +966,10 @@ abstract interface class VmServiceInterface {
   /// Perfettofile, an [RPCError] with error code `114`, `invalid timeline
   /// request`, will be returned as timeline events are written directly to a
   /// file, and thus cannot be retrieved through the VM Service, in these modes.
-  Future<Timeline> getVMTimeline(
-      {int? timeOriginMicros, int? timeExtentMicros});
+  Future<Timeline> getVMTimeline({
+    int? timeOriginMicros,
+    int? timeExtentMicros,
+  });
 
   /// The `getVMTimelineFlags` RPC returns information about the current VM
   /// timeline configuration.
@@ -1016,8 +1032,11 @@ abstract interface class VmServiceInterface {
   /// of relative paths, but this is not guaranteed.
   ///
   /// See [UriList].
-  Future<UriList> lookupResolvedPackageUris(String isolateId, List<String> uris,
-      {bool? local});
+  Future<UriList> lookupResolvedPackageUris(
+    String isolateId,
+    List<String> uris, {
+    bool? local,
+  });
 
   /// The `lookupPackageUris` RPC is used to convert a list of URIs to their
   /// unresolved paths. For example, URIs passed to this RPC are mapped in the
@@ -1130,8 +1149,11 @@ abstract interface class VmServiceInterface {
   ///
   /// This method will throw a [SentinelException] in the case a [Sentinel] is
   /// returned.
-  Future<Success> resume(String isolateId,
-      {/*StepOption*/ String? step, int? frameIndex});
+  Future<Success> resume(
+    String isolateId, {
+    /*StepOption*/ String? step,
+    int? frameIndex,
+  });
 
   /// The `setBreakpointState` RPC allows for breakpoints to be enabled or
   /// disabled, without requiring for the breakpoint to be completely removed.
@@ -1143,7 +1165,10 @@ abstract interface class VmServiceInterface {
   ///
   /// See [Breakpoint].
   Future<Breakpoint> setBreakpointState(
-      String isolateId, String breakpointId, bool enable);
+    String isolateId,
+    String breakpointId,
+    bool enable,
+  );
 
   /// The `setExceptionPauseMode` RPC is used to control if an isolate pauses
   /// when an exception is thrown.
@@ -1161,7 +1186,9 @@ abstract interface class VmServiceInterface {
   /// returned.
   @Deprecated('Use setIsolatePauseMode instead')
   Future<Success> setExceptionPauseMode(
-      String isolateId, /*ExceptionPauseMode*/ String mode);
+    String isolateId,
+    /*ExceptionPauseMode*/ String mode,
+  );
 
   /// The `setIsolatePauseMode` RPC is used to control if or when an isolate
   /// will pause due to a change in execution state.
@@ -1180,9 +1207,11 @@ abstract interface class VmServiceInterface {
   ///
   /// This method will throw a [SentinelException] in the case a [Sentinel] is
   /// returned.
-  Future<Success> setIsolatePauseMode(String isolateId,
-      {/*ExceptionPauseMode*/ String? exceptionPauseMode,
-      bool? shouldPauseOnExit});
+  Future<Success> setIsolatePauseMode(
+    String isolateId, {
+    /*ExceptionPauseMode*/ String? exceptionPauseMode,
+    bool? shouldPauseOnExit,
+  });
 
   /// The `setFlag` RPC is used to set a VM flag at runtime. Returns an error if
   /// the named flag does not exist, the flag may not be set at runtime, or the
@@ -1222,7 +1251,10 @@ abstract interface class VmServiceInterface {
   /// This method will throw a [SentinelException] in the case a [Sentinel] is
   /// returned.
   Future<Success> setLibraryDebuggable(
-      String isolateId, String libraryId, bool isDebuggable);
+    String isolateId,
+    String libraryId,
+    bool isDebuggable,
+  );
 
   /// The `setName` RPC is used to change the debugging name for an isolate.
   ///
@@ -1250,7 +1282,10 @@ abstract interface class VmServiceInterface {
   /// This method will throw a [SentinelException] in the case a [Sentinel] is
   /// returned.
   Future<Success> setTraceClassAllocation(
-      String isolateId, String classId, bool enable);
+    String isolateId,
+    String classId,
+    bool enable,
+  );
 
   /// The `setVMName` RPC is used to change the debugging name for the vm.
   ///
@@ -1371,8 +1406,12 @@ class VmServerConnection {
   /// Pending service extension requests to this client by id.
   final _pendingServiceExtensionRequests = <dynamic, _PendingServiceRequest>{};
 
-  VmServerConnection(this._requestStream, this._responseSink,
-      this._serviceExtensionRegistry, this._serviceImplementation) {
+  VmServerConnection(
+    this._requestStream,
+    this._responseSink,
+    this._serviceExtensionRegistry,
+    this._serviceImplementation,
+  ) {
     _requestStream.listen(_delegateRequest, onDone: _doneCompleter.complete);
     done.then((_) {
       for (var sub in _streamSubscriptions.values) {
@@ -1387,7 +1426,8 @@ class VmServerConnection {
   /// We don't attempt to do any serialization or deserialization of the
   /// request or response in this case
   Future<Map<String, Object?>> _forwardServiceExtensionRequest(
-      Map<String, Object?> request) {
+    Map<String, Object?> request,
+  ) {
     final originalId = request['id'];
     request = Map<String, Object?>.of(request);
     // Modify the request ID to ensure we don't have conflicts between
@@ -1411,8 +1451,12 @@ class VmServerConnection {
       }
       final method = request['method'] as String?;
       if (method == null) {
-        throw RPCError(null, RPCErrorKind.kInvalidRequest.code,
-            'Invalid Request', request);
+        throw RPCError(
+          null,
+          RPCErrorKind.kInvalidRequest.code,
+          'Invalid Request',
+          request,
+        );
       }
       final params = request['params'] as Map<String, dynamic>?;
       late Response response;
@@ -1674,14 +1718,10 @@ class VmServerConnection {
           response = await _serviceImplementation.getVMTimelineMicros();
           break;
         case 'pause':
-          response = await _serviceImplementation.pause(
-            params!['isolateId'],
-          );
+          response = await _serviceImplementation.pause(params!['isolateId']);
           break;
         case 'kill':
-          response = await _serviceImplementation.kill(
-            params!['isolateId'],
-          );
+          response = await _serviceImplementation.kill(params!['isolateId']);
           break;
         case 'lookupResolvedPackageUris':
           response = await _serviceImplementation.lookupResolvedPackageUris(
@@ -1771,9 +1811,7 @@ class VmServerConnection {
           );
           break;
         case 'setVMName':
-          response = await _serviceImplementation.setVMName(
-            params!['name'],
-          );
+          response = await _serviceImplementation.setVMName(params!['name']);
           break;
         case 'setVMTimelineFlags':
           response = await _serviceImplementation.setVMTimelineFlags(
@@ -1818,10 +1856,7 @@ class VmServerConnection {
             _responseSink.add({
               'jsonrpc': '2.0',
               'method': 'streamNotify',
-              'params': {
-                'streamId': id,
-                'event': e.toJson(),
-              },
+              'params': {'streamId': id, 'event': e.toJson()},
             });
           });
           response = Success();
@@ -1835,22 +1870,31 @@ class VmServerConnection {
           if (registeredClient != null) {
             // Check for any client which has registered this extension, if we
             // have one then delegate the request to that client.
-            _responseSink.add(await registeredClient
-                ._forwardServiceExtensionRequest(request));
+            _responseSink.add(
+              await registeredClient._forwardServiceExtensionRequest(request),
+            );
             // Bail out early in this case, we are just acting as a proxy and
             // never get a `Response` instance.
             return;
           } else if (method.startsWith('ext.')) {
             // Remaining methods with `ext.` are assumed to be registered via
             // dart:developer, which the service implementation handles.
-            final args =
-                params == null ? null : Map<String, dynamic>.of(params);
+            final args = params == null
+                ? null
+                : Map<String, dynamic>.of(params);
             final isolateId = args?.remove('isolateId');
-            response = await _serviceImplementation.callServiceExtension(method,
-                isolateId: isolateId, args: args);
+            response = await _serviceImplementation.callServiceExtension(
+              method,
+              isolateId: isolateId,
+              args: args,
+            );
           } else {
-            throw RPCError(method, RPCErrorKind.kMethodNotFound.code,
-                'Method not found', request);
+            throw RPCError(
+              method,
+              RPCErrorKind.kMethodNotFound.code,
+              'Method not found',
+              request,
+            );
           }
       }
       _responseSink.add({

runtime/vm/json_stream.cc

@@ -313,11 +313,6 @@ void JSONStream::PrintValue(Metric* metric) {
   metric->PrintJSON(this);
 }
 
-void JSONStream::PrintValue(MessageQueue* queue) {
-  PrintCommaIfNeeded();
-  queue->PrintJSON(this);
-}
-
 void JSONStream::PrintValue(Isolate* isolate, bool ref) {
   PrintCommaIfNeeded();
   isolate->PrintJSON(this, ref);
@@ -375,12 +370,6 @@ void JSONStream::PrintProperty(const char* name, Metric* metric) {
   PrintValue(metric);
 }
 
-void JSONStream::PrintProperty(const char* name, MessageQueue* queue) {
-  PRIVATE_NAME_CHECK();
-  PrintPropertyName(name);
-  PrintValue(queue);
-}
-
 void JSONStream::PrintProperty(const char* name, Isolate* isolate) {
   PRIVATE_NAME_CHECK();
   PrintPropertyName(name);

runtime/vm/json_stream.h

@@ -23,7 +23,6 @@ class GrowableObjectArray;
 class Instance;
 class JSONArray;
 class JSONObject;
-class MessageQueue;
 class Metric;
 class Object;
 class Script;
@@ -261,7 +260,6 @@ class JSONStream : ValueObject {
   void PrintValue(TokenPosition tp);
   void PrintValue(const ServiceEvent* event);
   void PrintValue(Metric* metric);
-  void PrintValue(MessageQueue* queue);
   void PrintValue(Isolate* isolate, bool ref = true);
   void PrintValue(IsolateGroup* isolate, bool ref = true);
   void PrintValue(const TimelineEvent* timeline_event);
@@ -337,7 +335,6 @@ class JSONStream : ValueObject {
   void PrintProperty(const char* name, Breakpoint* bpt);
   void PrintProperty(const char* name, TokenPosition tp);
   void PrintProperty(const char* name, Metric* metric);
-  void PrintProperty(const char* name, MessageQueue* queue);
   void PrintProperty(const char* name, Isolate* isolate);
   void PrintProperty(const char* name, IsolateGroup* isolate_group);
   void PrintProperty(const char* name, Zone* zone);
@@ -451,9 +448,6 @@ class JSONObject : public ValueObject {
   void AddProperty(const char* name, Metric* metric) const {
     stream_->PrintProperty(name, metric);
   }
-  void AddProperty(const char* name, MessageQueue* queue) const {
-    stream_->PrintProperty(name, queue);
-  }
   void AddProperty(const char* name, Isolate* isolate) const {
     stream_->PrintProperty(name, isolate);
   }
@@ -524,7 +518,6 @@ class JSONArray : public ValueObject {
   void AddValue(TokenPosition tp) const { stream_->PrintValue(tp); }
   void AddValue(const ServiceEvent* event) const { stream_->PrintValue(event); }
   void AddValue(Metric* metric) const { stream_->PrintValue(metric); }
-  void AddValue(MessageQueue* queue) const { stream_->PrintValue(queue); }
   void AddValue(const TimelineEvent* timeline_event) const {
     stream_->PrintValue(timeline_event);
   }

runtime/vm/message.cc

@@ -8,7 +8,6 @@
 
 #include "vm/dart_api_state.h"
 #include "vm/dart_entry.h"
-#include "vm/json_stream.h"
 #include "vm/object.h"
 #include "vm/port.h"
 
@@ -65,25 +64,6 @@ Message::~Message() {
   }
 }
 
-intptr_t Message::Id() const {
-  // Messages are allocated on the C heap. Use the raw address as the id.
-  return reinterpret_cast<intptr_t>(this);
-}
-
-const char* Message::PriorityAsString(Priority priority) {
-  switch (priority) {
-    case kNormalPriority:
-      return "Normal";
-      break;
-    case kOOBPriority:
-      return "OOB";
-      break;
-    default:
-      UNIMPLEMENTED();
-      return nullptr;
-  }
-}
-
 MessageQueue::MessageQueue() {
   head_ = nullptr;
   tail_ = nullptr;
@@ -198,57 +178,4 @@ intptr_t MessageQueue::Length() const {
   return length;
 }
 
-Message* MessageQueue::FindMessageById(intptr_t id) {
-  MessageQueue::Iterator it(this);
-  while (it.HasNext()) {
-    Message* current = it.Next();
-    ASSERT(current != nullptr);
-    if (current->Id() == id) {
-      return current;
-    }
-  }
-  return nullptr;
-}
-
-void MessageQueue::PrintJSON(JSONStream* stream) {
-#ifndef PRODUCT
-  JSONArray messages(stream);
-
-  Object& msg_handler = Object::Handle();
-
-  MessageQueue::Iterator it(this);
-  intptr_t depth = 0;
-  while (it.HasNext()) {
-    Message* current = it.Next();
-    JSONObject message(&messages);
-    message.AddProperty("type", "Message");
-    message.AddPropertyF("name", "Isolate Message (%" Px ")", current->Id());
-    message.AddPropertyF("messageObjectId", "messages/%" Px "", current->Id());
-    message.AddProperty("size", current->Size());
-    message.AddProperty("index", depth++);
-    message.AddPropertyF("_destinationPort", "%" Pd64 "",
-                         static_cast<int64_t>(current->dest_port()));
-    message.AddProperty("_priority",
-                        Message::PriorityAsString(current->priority()));
-    // TODO(johnmccutchan): Move port -> handler map out of Dart and into the
-    // VM, that way we can lookup the handler without invoking Dart code.
-    msg_handler = DartLibraryCalls::LookupHandler(current->dest_port());
-    if (msg_handler.IsClosure()) {
-      // Grab function from closure.
-      msg_handler = Closure::Cast(msg_handler).function();
-    }
-    if (msg_handler.IsFunction()) {
-      const Function& function = Function::Cast(msg_handler);
-      message.AddProperty("handler", function);
-
-      const Script& script = Script::Handle(function.script());
-      if (!script.IsNull()) {
-        message.AddLocation(script, function.token_pos(),
-                            function.end_token_pos());
-      }
-    }
-  }
-#endif  // !PRODUCT
-}
-
 }  // namespace dart

runtime/vm/message.h

@@ -19,7 +19,6 @@ typedef int64_t Dart_Port;
 
 namespace dart {
 
-class JSONStream;
 class PersistentHandle;
 
 class Message {
@@ -126,10 +125,6 @@ class Message {
     }
   }
 
-  intptr_t Id() const;
-
-  static const char* PriorityAsString(Priority priority);
-
  private:
   static intptr_t const kPersistentHandleSnapshotLen = -1;
   static intptr_t const kFinalizerSnapshotLen = -2;
@@ -192,11 +187,6 @@ class MessageQueue {
 
   intptr_t Length() const;
 
-  // Returns the message with id or nullptr.
-  Message* FindMessageById(intptr_t id);
-
-  void PrintJSON(JSONStream* stream);
-
  private:
   Message* head_;
   Message* tail_;

runtime/vm/message_handler.cc

@@ -55,7 +55,6 @@ const char* MessageHandler::MessageStatusString(MessageStatus status) {
 MessageHandler::MessageHandler()
     : queue_(new MessageQueue()),
       oob_queue_(new MessageQueue()),
-      oob_message_handling_allowed_(true),
       paused_(0),
 #if !defined(PRODUCT)
       should_pause_on_start_(false),
@@ -282,9 +281,6 @@ MessageHandler::MessageStatus MessageHandler::HandleNextMessage() {
 }
 
 MessageHandler::MessageStatus MessageHandler::HandleOOBMessages() {
-  if (!oob_message_handling_allowed_) {
-    return kOK;
-  }
   MonitorLocker ml(&monitor_);
 #if defined(DEBUG)
   CheckAccess();
@@ -332,6 +328,12 @@ bool MessageHandler::HasMessages() {
   return !queue_->IsEmpty();
 }
 
+MessageHandler::MessageCount MessageHandler::GetMessageCounts() {
+  MonitorLocker ml(&monitor_);
+  return {.num_messages = queue_->Length(),
+          .num_oob_messages = queue_->Length()};
+}
+
 void MessageHandler::TaskCallback() {
   ASSERT(Isolate::Current() == nullptr);
   MessageStatus status = kOK;
@@ -540,15 +542,4 @@ void MessageHandler::PausedOnExitLocked(MonitorLocker* ml, bool paused) {
 }
 #endif  // !defined(PRODUCT)
 
-MessageHandler::AcquiredQueues::AcquiredQueues(MessageHandler* handler)
-    : handler_(handler), ml_(&handler->monitor_) {
-  ASSERT(handler != nullptr);
-  handler_->oob_message_handling_allowed_ = false;
-}
-
-MessageHandler::AcquiredQueues::~AcquiredQueues() {
-  ASSERT(handler_ != nullptr);
-  handler_->oob_message_handling_allowed_ = true;
-}
-
 }  // namespace dart

runtime/vm/message_handler.h

@@ -72,6 +72,13 @@ class MessageHandler : public PortHandler {
   // handler.
   bool HasMessages();
 
+  struct MessageCount {
+    intptr_t num_messages;
+    intptr_t num_oob_messages;
+  };
+
+  MessageCount GetMessageCounts();
+
   // Whether to keep this message handler alive or whether it should shutdown.
   virtual bool KeepAliveLocked() { return true; }
 
@@ -109,36 +116,6 @@ class MessageHandler : public PortHandler {
   void PausedOnExit(bool paused);
 #endif
 
-  // Gives temporary ownership of |queue| and |oob_queue|. Using this object
-  // has the side effect that no OOB messages will be handled if a stack
-  // overflow interrupt is delivered.
-  class AcquiredQueues : public ValueObject {
-   public:
-    explicit AcquiredQueues(MessageHandler* handler);
-
-    ~AcquiredQueues();
-
-    MessageQueue* queue() {
-      if (handler_ == nullptr) {
-        return nullptr;
-      }
-      return handler_->queue_;
-    }
-
-    MessageQueue* oob_queue() {
-      if (handler_ == nullptr) {
-        return nullptr;
-      }
-      return handler_->oob_queue_;
-    }
-
-   private:
-    MessageHandler* handler_;
-    SafepointMonitorLocker ml_;
-
-    friend class MessageHandler;
-  };
-
  protected:
   // Custom message notification.  Optionally provided by subclass.
   virtual void MessageNotify(Message::Priority priority);
@@ -223,9 +200,6 @@ class MessageHandler : public PortHandler {
   Monitor monitor_;  // Protects all fields in MessageHandler.
   MessageQueue* queue_;
   MessageQueue* oob_queue_;
-  // This flag is not thread safe and can only reliably be accessed on a single
-  // thread.
-  bool oob_message_handling_allowed_;
 
   // Only accessed by [PortMap], protected by [PortMap]s lock. See ports()
   // getter.

runtime/vm/message_handler_test.cc

@@ -162,23 +162,15 @@ VM_UNIT_TEST_CASE(MessageHandler_HasOOBMessages) {
   std::unique_ptr<Message> message = BlankMessage(1, Message::kNormalPriority);
   handler_peer.PostMessage(std::move(message));
   EXPECT(!handler.HasOOBMessages());
-  {
-    // Acquire ownership of message handler queues, verify one regular message.
-    MessageHandler::AcquiredQueues aq(&handler);
-    EXPECT(aq.queue()->Length() == 1);
-  }
+  EXPECT_EQ(1, handler.GetMessageCounts().num_messages);
 
   // Post an oob message.
   message = BlankMessage(1, Message::kOOBPriority);
   handler_peer.PostMessage(std::move(message));
   EXPECT(handler.HasOOBMessages());
-  {
-    // Acquire ownership of message handler queues, verify one regular and one
-    // OOB message.
-    MessageHandler::AcquiredQueues aq(&handler);
-    EXPECT(aq.queue()->Length() == 1);
-    EXPECT(aq.oob_queue()->Length() == 1);
-  }
+  const auto count = handler.GetMessageCounts();
+  EXPECT_EQ(1, count.num_messages);
+  EXPECT_EQ(1, count.num_oob_messages);
 
   // Delete all pending messages.
   handler_peer.OnAllPortsClosed();
@@ -285,11 +277,7 @@ VM_UNIT_TEST_CASE(MessageHandler_HandleNextMessage_Shutdown) {
   Dart_Port* ports = handler.port_buffer();
   EXPECT_EQ(port2, ports[0]);  // oob_message1, ok
   EXPECT_EQ(port3, ports[1]);  // oob_message2, shutdown
-  {
-    // The oob queue has been cleared.  oob_message3 is gone.
-    MessageHandler::AcquiredQueues aq(&handler);
-    EXPECT(aq.oob_queue()->Length() == 0);
-  }
+  EXPECT(!handler.HasOOBMessages());
   handler_peer.OnAllPortsClosed();
 }
 

runtime/vm/message_test.cc

@@ -54,13 +54,6 @@ TEST_CASE(MessageQueue_BasicOperations) {
   EXPECT(it.Next() == msg2);
   EXPECT(!it.HasNext());
 
-  // Lookup messages by id.
-  EXPECT(queue.FindMessageById(reinterpret_cast<intptr_t>(msg1)) == msg1);
-  EXPECT(queue.FindMessageById(reinterpret_cast<intptr_t>(msg2)) == msg2);
-
-  // Lookup bad id.
-  EXPECT(queue.FindMessageById(0x1) == nullptr);
-
   // Remove message 1
   msg = queue.Dequeue();
   EXPECT(msg != nullptr);

runtime/vm/object_test.cc

@@ -3868,10 +3868,7 @@ static void Finalizer_PreserveOne(Thread* thread,
 #endif
 
   MessageHandler* handler = thread->isolate()->message_handler();
-  {
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT_EQ(0, handler->GetMessageCounts().num_messages);
 
   const auto& finalizer = Finalizer::Handle(Finalizer::New(space));
   finalizer.set_isolate(thread->isolate());
@@ -3905,11 +3902,7 @@ static void Finalizer_PreserveOne(Thread* thread,
             NumEntries(FinalizerEntry::Handle(finalizer.entries_collected())));
 
   // We should have no messages.
-  {
-    // Acquire ownership of message handler queues.
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT(!handler->HasMessages());
 }
 
 ISOLATE_UNIT_TEST_CASE(Finalizer_PreserveNoDetachOne_NewSpace) {
@@ -3934,10 +3927,7 @@ static void Finalizer_ClearDetachOne(Thread* thread, Heap::Space space) {
 #endif
 
   MessageHandler* handler = thread->isolate()->message_handler();
-  {
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT(!handler->HasMessages());
 
   const auto& finalizer = Finalizer::Handle(Finalizer::New(space));
   finalizer.set_isolate(thread->isolate());
@@ -3970,11 +3960,7 @@ static void Finalizer_ClearDetachOne(Thread* thread, Heap::Space space) {
             NumEntries(FinalizerEntry::Handle(finalizer.entries_collected())));
 
   // We should have no messages.
-  {
-    // Acquire ownership of message handler queues.
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT(!handler->HasMessages());
 }
 
 ISOLATE_UNIT_TEST_CASE(Finalizer_ClearDetachOne_NewSpace) {
@@ -3993,10 +3979,7 @@ static void Finalizer_ClearValueOne(Thread* thread,
 #endif
 
   MessageHandler* handler = thread->isolate()->message_handler();
-  {
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT(!handler->HasMessages());
 
   const auto& finalizer = Finalizer::Handle(Finalizer::New(space));
   finalizer.set_isolate(thread->isolate());
@@ -4034,11 +4017,7 @@ static void Finalizer_ClearValueOne(Thread* thread,
             NumEntries(FinalizerEntry::Handle(finalizer.entries_collected())));
 
   // We should have 1 message.
-  {
-    // Acquire ownership of message handler queues.
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(1, aq.queue()->Length());
-  }
+  EXPECT_EQ(1, handler->GetMessageCounts().num_messages);
 }
 
 ISOLATE_UNIT_TEST_CASE(Finalizer_ClearValueOne_NewSpace) {
@@ -4065,10 +4044,7 @@ static void Finalizer_DetachOne(Thread* thread,
 #endif
 
   MessageHandler* handler = thread->isolate()->message_handler();
-  {
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT(!handler->HasMessages());
 
   const auto& finalizer = Finalizer::Handle(Finalizer::New(space));
   finalizer.set_isolate(thread->isolate());
@@ -4106,11 +4082,7 @@ static void Finalizer_DetachOne(Thread* thread,
             NumEntries(FinalizerEntry::Handle(finalizer.entries_collected())));
 
   // We should have no message.
-  {
-    // Acquire ownership of message handler queues.
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT(!handler->HasMessages());
 }
 
 ISOLATE_UNIT_TEST_CASE(Finalizer_DetachOne_NewSpace) {
@@ -4135,10 +4107,7 @@ static void Finalizer_GcFinalizer(Thread* thread, Heap::Space space) {
 #endif
 
   MessageHandler* handler = thread->isolate()->message_handler();
-  {
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT(!handler->HasMessages());
 
   const auto& detach = String::Handle(OneByteString::New("detach", space));
   const auto& token = String::Handle(OneByteString::New("token", space));
@@ -4162,11 +4131,7 @@ static void Finalizer_GcFinalizer(Thread* thread, Heap::Space space) {
   }
 
   // We should have no message, the Finalizer itself has been GCed.
-  {
-    // Acquire ownership of message handler queues.
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(0, aq.queue()->Length());
-  }
+  EXPECT(!handler->HasMessages());
 }
 
 ISOLATE_UNIT_TEST_CASE(Finalizer_GcFinalizer_NewSpace) {
@@ -4194,11 +4159,7 @@ static void Finalizer_TwoEntriesCrossGen(
   MessageHandler* handler = thread->isolate()->message_handler();
   // We're reusing the isolate in a loop, so there are messages from previous
   // runs of this test.
-  intptr_t queue_length_start = 0;
-  {
-    MessageHandler::AcquiredQueues aq(handler);
-    queue_length_start = aq.queue()->Length();
-  }
+  const intptr_t queue_length_start = handler->GetMessageCounts().num_messages;
 
   const auto& finalizer = Finalizer::Handle(Finalizer::New(spaces[0]));
   finalizer.set_isolate(thread->isolate());
@@ -4270,11 +4231,8 @@ static void Finalizer_TwoEntriesCrossGen(
             NumEntries(FinalizerEntry::Handle(finalizer.entries_collected())));
 
   const intptr_t expect_num_messages = expect_num_cleared == 0 ? 0 : 1;
-  {
-    // Acquire ownership of message handler queues.
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(expect_num_messages + queue_length_start, aq.queue()->Length());
-  }
+  EXPECT_EQ(expect_num_messages + queue_length_start,
+            handler->GetMessageCounts().num_messages);
 }
 
 const intptr_t kFinalizerTwoEntriesNumObjects = 9;
@@ -4991,11 +4949,7 @@ static void NativeFinalizer_TwoEntriesCrossGen(
   MessageHandler* handler = thread->isolate()->message_handler();
   // We're reusing the isolate in a loop, so there are messages from previous
   // runs of this test.
-  intptr_t queue_length_start = 0;
-  {
-    MessageHandler::AcquiredQueues aq(handler);
-    queue_length_start = aq.queue()->Length();
-  }
+  const intptr_t queue_length_start = handler->GetMessageCounts().num_messages;
 
   const auto& callback = Pointer::Handle(Pointer::New(
       reinterpret_cast<uword>(&NativeFinalizer_TwoEntriesCrossGen_Finalizer),
@@ -5104,11 +5058,8 @@ static void NativeFinalizer_TwoEntriesCrossGen(
   EXPECT_EQ(clear_value_2 ? 1 : 0, token2_memory);
 
   const intptr_t expect_num_messages = expect_num_cleared == 0 ? 0 : 1;
-  {
-    // Acquire ownership of message handler queues.
-    MessageHandler::AcquiredQueues aq(handler);
-    EXPECT_EQ(expect_num_messages + queue_length_start, aq.queue()->Length());
-  }
+  EXPECT_EQ(expect_num_messages + queue_length_start,
+            handler->GetMessageCounts().num_messages);
 
   // Simulate detachments.
   entry1.set_token(entry1);

runtime/vm/service.cc

@@ -1755,8 +1755,8 @@ static void GetStack(Thread* thread, JSONStream* js) {
   jsobj.AddProperty("truncated", truncated);
 
   {
-    MessageHandler::AcquiredQueues aq(isolate->message_handler());
-    jsobj.AddProperty("messages", aq.queue());
+    // Deprecated and always empty since protocol version 4.22.
+    JSONArray jsarr(&jsobj, "messages");
   }
 }
 
@@ -2209,29 +2209,6 @@ static ObjectPtr LookupHeapObjectCode(char** parts, int num_parts) {
   return Object::sentinel().ptr();
 }
 
-static ObjectPtr LookupHeapObjectMessage(Thread* thread,
-                                         char** parts,
-                                         int num_parts) {
-  if (num_parts != 2) {
-    return Object::sentinel().ptr();
-  }
-  uword message_id = 0;
-  if (!GetUnsignedIntegerId(parts[1], &message_id, 16)) {
-    return Object::sentinel().ptr();
-  }
-  MessageHandler::AcquiredQueues aq(thread->isolate()->message_handler());
-  Message* message = aq.queue()->FindMessageById(message_id);
-  if (message == nullptr) {
-    // The user may try to load an expired message.
-    return Object::sentinel().ptr();
-  }
-  if (message->IsRaw()) {
-    return message->raw_obj();
-  } else {
-    return ReadMessage(thread, message);
-  }
-}
-
 static ObjectPtr LookupHeapObject(Thread* thread,
                                   const char* id_original,
                                   ObjectIdRing::LookupResult* result) {
@@ -2284,8 +2261,6 @@ static ObjectPtr LookupHeapObject(Thread* thread,
     return LookupHeapObjectTypeArguments(thread, parts, num_parts);
   } else if (strcmp(parts[0], "code") == 0) {
     return LookupHeapObjectCode(parts, num_parts);
-  } else if (strcmp(parts[0], "messages") == 0) {
-    return LookupHeapObjectMessage(thread, parts, num_parts);
   }
 
   // Not found.

runtime/vm/service/service.md

@@ -1,8 +1,8 @@
-# Dart VM Service Protocol 4.21
+# Dart VM Service Protocol 4.22
 
 > Please post feedback to the [observatory-discuss group][discuss-list]
 
-This document describes of _version 4.20_ of the Dart VM Service Protocol. This
+This document describes of _version 4.22_ of the Dart VM Service Protocol. This
 protocol is used to communicate with a running Dart Virtual Machine.
 
 To use the Service Protocol, start the VM with the *--observe* flag.
@@ -679,7 +679,7 @@ Success invalidateIdZone(string isolateId, string idZoneId)
 ```
 
 The _invalidateIdZone_ RPC is used to invalidate all the IDs that have been
-allocated in a certain ID zone. Invaliding the IDs makes them expire. See 
+allocated in a certain ID zone. Invaliding the IDs makes them expire. See
 [IDs and Names](#ids-and-names) for more information.
 
 ### invoke
@@ -4058,6 +4058,9 @@ class Message extends Response {
 A _Message_ provides information about a pending isolate message and the
 function that will be invoked to handle it.
 
+This type is deprecated starting with protocol version 4.22 and instances of
+this type will not be returned in protocol responses.
+
 ### Microtask
 
 ```
@@ -4713,6 +4716,9 @@ class Stack extends Response {
   // different encoding.
   Frame[] awaiterFrames [optional];
 
+  // Deprecated since version 4.22 of the protocol. Will always be empty
+  // in the response.
+  //
   // A list of messages in the isolate's message queue.
   Message[] messages;
 
@@ -5073,5 +5079,7 @@ version | comments
 4.18 | Added `Microtask` timeline stream.
 4.19 | Added `getQueuedMicrotasks` RPC, added `Microtask` and `QueuedMicrotasks` types, and added RPC error 115 "Cannot get queued microtasks".
 4.20 | Deprecated `streamCpuSamplesWithUserTag` RPC.
+4.21 | Added `InstanceKind.Pointer`.
+4.22 | Deprecated `messages` property on `Stack`. Deprecated `Message` type.
 
 [discuss-list]: https://groups.google.com/a/dartlang.org/forum/#!forum/observatory-discuss