[vm] Fix race in NativeMessageHandler

Introduce `NativeMessageHandler::Cleanup` which
waits for all pending `NativeMessageHandler`
deletions to complete.

Fixes #56744

TEST=hard to distill this specific race into a test

Change-Id: Id6196f955ff7a874c07d3a11f766d74accbde96c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/385780
Reviewed-by: Ryan Macnak <[email protected]>
Commit-Queue: Slava Egorov <[email protected]>

runtime/vm/dart.cc

@@ -34,6 +34,7 @@
 #include "vm/message_handler.h"
 #include "vm/metrics.h"
 #include "vm/native_entry.h"
+#include "vm/native_message_handler.h"
 #include "vm/object.h"
 #include "vm/object_id_ring.h"
 #include "vm/object_store.h"
@@ -337,6 +338,7 @@ char* Dart::DartInit(const Dart_InitializeParams* params) {
   Isolate::InitVM();
   UserTags::Init();
   PortMap::Init();
+  NativeMessageHandler::Init();
   Service::Init();
   FreeListElement::Init();
   ForwardingCorpse::Init();
@@ -702,6 +704,8 @@ char* Dart::Cleanup() {
                  UptimeMillis());
   }
   DartInitializationState::SetUnInitialized();
+
+  NativeMessageHandler::Cleanup();
   PortMap::Shutdown();
   thread_pool_->Shutdown();
   delete thread_pool_;

runtime/vm/native_message_handler.cc

@@ -15,6 +15,9 @@
 
 namespace dart {
 
+Monitor* NativeMessageHandler::monitor_ = nullptr;
+intptr_t NativeMessageHandler::pending_deletions_ = 0;
+
 NativeMessageHandler::NativeMessageHandler(const char* name,
                                            Dart_NativeMessageHandler func,
                                            intptr_t max_concurrency)
@@ -61,11 +64,48 @@ void NativeMessageHandler::PostMessage(std::unique_ptr<Message> message,
 }
 
 void NativeMessageHandler::RequestDeletion(NativeMessageHandler* handler) {
-  ThreadPool::RequestShutdown(&handler->pool_, [handler]() { delete handler; });
+  {
+    MonitorLocker ml(monitor_);
+    pending_deletions_++;
+  }
+
+  ThreadPool::RequestShutdown(&handler->pool_, [handler]() {
+    delete handler;
+
+    // Once the handler and its pool is gone make sure to wake up
+    // |NativeMessageHandler::Cleanup| which might be waiting.
+    {
+      MonitorLocker ml(monitor_);
+      pending_deletions_--;
+      if (pending_deletions_ == 0) {
+        ml.Notify();
+      }
+    }
+  });
 }
 
 void NativeMessageHandler::Shutdown() {
   pool_.Shutdown();
 }
 
+void NativeMessageHandler::Init() {
+  monitor_ = new Monitor();
+}
+
+void NativeMessageHandler::Cleanup() {
+  {
+    MonitorLocker ml(monitor_);
+    // By the time we get here we don't really expect new deletions to be
+    // requested. We proceed with VM shutdown once we have no pending deletions.
+    // In other words words don't try to guard against a race between
+    // |Dart_CloseNativePort| and |Dart_Cleanup| - that's considered an API
+    // misuse.
+    while (pending_deletions_ > 0) {
+      ml.Wait();
+    }
+  }
+  delete monitor_;
+  monitor_ = nullptr;
+}
+
 }  // namespace dart

runtime/vm/native_message_handler.h

@@ -47,17 +47,28 @@ class NativeMessageHandler final : public PortHandler {
   // running Dart_NativeMessageHandler callbacks. No new callbacks will be
   // scheduled after this call.
   //
-  // Note: |handler| might be deleted synchronously if no callback is running,
+  // |handler| might be deleted synchronously if no callback is running,
   // or it can be deleted later on a worker thread.
+  //
+  // |RequestDeletion| should be called after |Init| but before |Cleanup|.
+  // |Cleanup| will wait for all pending deletions to complete - which allows
+  // VM to shutdown cleanly.
   static void RequestDeletion(NativeMessageHandler* handler);
 
   void Shutdown() override;
 
+  static void Init();
+
+  static void Cleanup();
+
  private:
   PortSet<PortSetEntry>* ports(PortMap::Locker& locker) override {
     return nullptr;
   }
 
+  static Monitor* monitor_;
+  static intptr_t pending_deletions_;
+
   CStringUniquePtr name_;
   const Dart_NativeMessageHandler func_;