[vm] Fix race calling Dart_CloseNativePort during/after Dart_Cleanup.

Fix race between service/kernel isolate startup and Dart_Init. TEST=DartAPI_InvokeVMServiceMethod[_Loop] under rr chaos mode Bug: #57096 Change-Id: I50a26d61efa9106ce4bff0b268e68088570f97d0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/397984 Commit-Queue: Ryan Macnak <[email protected]> Reviewed-by: Slava Egorov <[email protected]>
dart-lang · Dec 2, 2024 · 3feb138 · 3feb138
1 parent dcb9ec1
commit 3feb138
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 30 deletions.
diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc
@@ -501,27 +501,6 @@ char* Dart::DartInit(const Dart_InitializeParams* params) {
   Isolate::SetRegisterKernelBlobCallback(params->register_kernel_blob);
   Isolate::SetUnregisterKernelBlobCallback(params->unregister_kernel_blob);
 
-#ifndef PRODUCT
-  const bool support_service = true;
-  Service::SetGetServiceAssetsCallback(params->get_service_assets);
-#else
-  const bool support_service = false;
-#endif
-
-  const bool is_dart2_aot_precompiler =
-      FLAG_precompiled_mode && !kDartPrecompiledRuntime;
-
-  if (!is_dart2_aot_precompiler &&
-      (support_service || !kDartPrecompiledRuntime)) {
-    ServiceIsolate::Run();
-  }
-
-#ifndef DART_PRECOMPILED_RUNTIME
-  if (params->start_kernel_isolate) {
-    KernelIsolate::InitializeState();
-  }
-#endif  // DART_PRECOMPILED_RUNTIME
-
   return nullptr;
 }
 
@@ -538,6 +517,21 @@ char* Dart::Init(const Dart_InitializeParams* params) {
     return retval;
   }
   DartInitializationState::SetInitialized();
+
+  // The service and kernel isolates require the VM state to be initialized.
+  // The embedder, not the VM, should trigger creation of the service and kernel
+  // isolates. https://github.com/dart-lang/sdk/issues/33433
+#if !defined(PRODUCT)
+  Service::SetGetServiceAssetsCallback(params->get_service_assets);
+  ServiceIsolate::Run();
+#endif
+
+#if !defined(DART_PRECOMPILED_RUNTIME)
+  if (params->start_kernel_isolate) {
+    KernelIsolate::InitializeState();
+  }
+#endif
+
   return nullptr;
 }
 

diff --git a/runtime/vm/native_api_impl.cc b/runtime/vm/native_api_impl.cc
@@ -43,6 +43,26 @@ class IsolateLeaveScope {
   DISALLOW_COPY_AND_ASSIGN(IsolateLeaveScope);
 };
 
+class DartApiCallScope : public ValueObject {
+ public:
+  DartApiCallScope() : active_(Dart::SetActiveApiCall()) {}
+  ~DartApiCallScope() {
+    if (active_) {
+      Dart::ResetActiveApiCall();
+    }
+  }
+  bool active() const { return active_; }
+
+ private:
+  const bool active_;
+};
+
+#define ENTER_API_CALL_OR_RETURN(failure_value)                                \
+  DartApiCallScope api_call_scope;                                             \
+  if (!api_call_scope.active()) {                                              \
+    return failure_value;                                                      \
+  }
+
 static bool PostCObjectHelper(Dart_Port port_id, Dart_CObject* message) {
   AllocOnlyStackZone zone;
   std::unique_ptr<Message> msg = WriteApiMessage(
@@ -89,20 +109,19 @@ Dart_NewConcurrentNativePort(const char* name,
                  CURRENT_FUNC);
     return ILLEGAL_PORT;
   }
-  if (!Dart::SetActiveApiCall()) {
-    return ILLEGAL_PORT;
-  }
+  ENTER_API_CALL_OR_RETURN(ILLEGAL_PORT);
   // Start the native port without a current isolate.
   IsolateLeaveScope saver(Isolate::Current());
 
   NativeMessageHandler* nmh =
       new NativeMessageHandler(name, handler, max_concurrency);
   Dart_Port port_id = PortMap::CreatePort(nmh);
-  Dart::ResetActiveApiCall();
   return port_id;
 }
 
 DART_EXPORT bool Dart_CloseNativePort(Dart_Port native_port_id) {
+  ENTER_API_CALL_OR_RETURN(false)
+
   // Close the native port without a current isolate.
   IsolateLeaveScope saver(Isolate::Current());
 
@@ -121,15 +140,16 @@ DART_EXPORT bool Dart_InvokeVMServiceMethod(uint8_t* request_json,
                                             intptr_t* response_json_length,
                                             char** error) {
 #if !defined(PRODUCT)
-  Isolate* isolate = Isolate::Current();
-  ASSERT(isolate == nullptr || !isolate->is_service_isolate());
-  IsolateLeaveScope saver(isolate);
-
-  if (!Dart::IsInitialized()) {
+  DartApiCallScope api_call;
+  if (!api_call.active()) {
     *error = ::dart::Utils::StrDup("VM Service is not active.");
     return false;
   }
 
+  Isolate* isolate = Isolate::Current();
+  ASSERT(isolate == nullptr || !isolate->is_service_isolate());
+  IsolateLeaveScope saver(isolate);
+
   // We only allow one isolate reload at a time.  If this turns out to be on the
   // critical path, we can change it to have a global datastructure which is
   // mapping the reply ports to receive buffers.