[vm] Prevent GC when there are outstanding pointers from Dart_TypedDataAcquireData.

Bug: https://github.com/dart-lang/sdk/issues/37256
Change-Id: I1dcd2e32d8308c5a7169731169a048b8f1bfcc79
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106023
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>

runtime/vm/compiler/frontend/kernel_translation_helper.cc

@@ -499,10 +499,9 @@ RawLibrary* TranslationHelper::LookupLibraryByKernelLibrary(
   ASSERT(IsLibrary(kernel_library) ||
          IsAdministrative(CanonicalNameParent(kernel_library)));
   {
-    NoSafepointScope no_safepoint_scope(thread_);
-    RawLibrary* raw_lib;
     name_index_handle_ = Smi::New(kernel_library);
-    raw_lib = info_.LookupLibrary(thread_, name_index_handle_);
+    RawLibrary* raw_lib = info_.LookupLibrary(thread_, name_index_handle_);
+    NoSafepointScope no_safepoint_scope(thread_);
     if (raw_lib != Library::null()) {
       return raw_lib;
     }
@@ -521,10 +520,9 @@ RawLibrary* TranslationHelper::LookupLibraryByKernelLibrary(
 RawClass* TranslationHelper::LookupClassByKernelClass(NameIndex kernel_class) {
   ASSERT(IsClass(kernel_class));
   {
-    NoSafepointScope no_safepoint_scope(thread_);
-    RawClass* raw_class;
     name_index_handle_ = Smi::New(kernel_class);
-    raw_class = info_.LookupClass(thread_, name_index_handle_);
+    RawClass* raw_class = info_.LookupClass(thread_, name_index_handle_);
+    NoSafepointScope no_safepoint_scope(thread_);
     if (raw_class != Class::null()) {
       return raw_class;
     }

runtime/vm/dart_api_impl.cc

@@ -1465,13 +1465,14 @@ DART_EXPORT Dart_Handle Dart_GetStickyError() {
   Thread* T = Thread::Current();
   Isolate* I = T->isolate();
   CHECK_ISOLATE(I);
-  NoSafepointScope no_safepoint_scope;
-  if (I->sticky_error() != Error::null()) {
-    TransitionNativeToVM transition(T);
-    Dart_Handle error = Api::NewHandle(T, I->sticky_error());
-    return error;
+  {
+    NoSafepointScope no_safepoint_scope;
+    if (I->sticky_error() == Error::null()) {
+      return Api::Null();
+    }
   }
-  return Dart_Null();
+  TransitionNativeToVM transition(T);
+  return Api::NewHandle(T, I->sticky_error());
 }
 
 DART_EXPORT void Dart_NotifyIdle(int64_t deadline) {
@@ -3695,7 +3696,8 @@ DART_EXPORT Dart_Handle Dart_TypedDataAcquireData(Dart_Handle object,
   intptr_t size_in_bytes = 0;
   void* data_tmp = NULL;
   bool external = false;
-  // If it is an external typed data object just return the data field.
+  T->IncrementNoSafepointScopeDepth();
+  START_NO_CALLBACK_SCOPE(T);
   if (RawObject::IsExternalTypedDataClassId(class_id)) {
     const ExternalTypedData& obj =
         Api::UnwrapExternalTypedDataHandle(Z, object);
@@ -3705,13 +3707,10 @@ DART_EXPORT Dart_Handle Dart_TypedDataAcquireData(Dart_Handle object,
     data_tmp = obj.DataAddr(0);
     external = true;
   } else if (RawObject::IsTypedDataClassId(class_id)) {
-    // Regular typed data object, set up some GC and API callback guards.
     const TypedData& obj = Api::UnwrapTypedDataHandle(Z, object);
     ASSERT(!obj.IsNull());
     length = obj.Length();
     size_in_bytes = length * TypedData::ElementSizeInBytes(class_id);
-    T->IncrementNoSafepointScopeDepth();
-    START_NO_CALLBACK_SCOPE(T);
     data_tmp = obj.DataAddr(0);
   } else {
     ASSERT(RawObject::IsTypedDataViewClassId(class_id));
@@ -3724,8 +3723,6 @@ DART_EXPORT Dart_Handle Dart_TypedDataAcquireData(Dart_Handle object,
     val = view_obj.offset_in_bytes();
     intptr_t offset_in_bytes = val.Value();
     const auto& obj = Instance::Handle(view_obj.typed_data());
-    T->IncrementNoSafepointScopeDepth();
-    START_NO_CALLBACK_SCOPE(T);
     if (TypedData::IsTypedData(obj)) {
       const TypedData& data_obj = TypedData::Cast(obj);
       data_tmp = data_obj.DataAddr(offset_in_bytes);
@@ -3769,10 +3766,8 @@ DART_EXPORT Dart_Handle Dart_TypedDataReleaseData(Dart_Handle object) {
       !RawObject::IsTypedDataClassId(class_id)) {
     RETURN_TYPE_ERROR(Z, object, 'TypedData');
   }
-  if (!RawObject::IsExternalTypedDataClassId(class_id)) {
-    T->DecrementNoSafepointScopeDepth();
-    END_NO_CALLBACK_SCOPE(T);
-  }
+  T->DecrementNoSafepointScopeDepth();
+  END_NO_CALLBACK_SCOPE(T);
   if (FLAG_verify_acquired_data) {
     const Object& obj = Object::Handle(Z, Api::UnwrapHandle(object));
     WeakTable* table = I->api_state()->acquired_table();

runtime/vm/dart_api_impl_test.cc

@@ -2337,6 +2337,7 @@ static void TestDirectAccess(Dart_Handle lib,
   void* data;
   intptr_t len;
   result = Dart_TypedDataAcquireData(array, &type, &data, &len);
+  EXPECT(!Thread::Current()->IsAtSafepoint());
   EXPECT_VALID(result);
   EXPECT_EQ(expected_type, type);
   EXPECT_EQ(kLength, len);
@@ -2345,14 +2346,12 @@ static void TestDirectAccess(Dart_Handle lib,
     EXPECT_EQ(i, dataP[i]);
   }
 
-  if (!is_external) {
-    // Now try allocating a string with outstanding Acquires and it should
-    // return an error.
-    result = NewString("We expect an error here");
-    EXPECT_ERROR(result,
-                 "Internal Dart data pointers have been acquired, "
-                 "please release them using Dart_TypedDataReleaseData.");
-  }
+  // Now try allocating a string with outstanding Acquires and it should
+  // return an error.
+  result = NewString("We expect an error here");
+  EXPECT_ERROR(result,
+               "Internal Dart data pointers have been acquired, "
+               "please release them using Dart_TypedDataReleaseData.");
 
   // Now modify the values in the directly accessible array and then check
   // it we see the changes back in dart.
@@ -2361,6 +2360,7 @@ static void TestDirectAccess(Dart_Handle lib,
   }
 
   // Release direct access to the typed data object.
+  EXPECT(!Thread::Current()->IsAtSafepoint());
   result = Dart_TypedDataReleaseData(array);
   EXPECT_VALID(result);
 
@@ -2369,6 +2369,29 @@ static void TestDirectAccess(Dart_Handle lib,
   EXPECT_VALID(result);
 }
 
+class BackgroundGCTask : public ThreadPool::Task {
+ public:
+  BackgroundGCTask(Isolate* isolate, Monitor* monitor, bool* done)
+      : isolate_(isolate), monitor_(monitor), done_(done) {}
+  virtual void Run() {
+    Thread::EnterIsolateAsHelper(isolate_, Thread::kUnknownTask);
+    for (intptr_t i = 0; i < 10; i++) {
+      Thread::Current()->heap()->CollectAllGarbage(Heap::kDebugging);
+    }
+    Thread::ExitIsolateAsHelper();
+    {
+      MonitorLocker ml(monitor_);
+      *done_ = true;
+      ml.Notify();
+    }
+  }
+
+ private:
+  Isolate* isolate_;
+  Monitor* monitor_;
+  bool* done_;
+};
+
 static void TestTypedDataDirectAccess1() {
   const char* kScriptChars =
       "import 'dart:typed_data';\n"
@@ -2397,20 +2420,35 @@ static void TestTypedDataDirectAccess1() {
   // Create a test library and Load up a test script in it.
   Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
 
-  // Test with an regular typed data object.
-  Dart_Handle list_access_test_obj;
-  list_access_test_obj = Dart_Invoke(lib, NewString("main"), 0, NULL);
-  EXPECT_VALID(list_access_test_obj);
-  TestDirectAccess(lib, list_access_test_obj, Dart_TypedData_kInt8, false);
+  Monitor monitor;
+  bool done = false;
+  Dart::thread_pool()->Run<BackgroundGCTask>(Isolate::Current(), &monitor,
+                                             &done);
 
-  // Test with an external typed data object.
-  uint8_t data[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
-  intptr_t data_length = ARRAY_SIZE(data);
-  Dart_Handle ext_list_access_test_obj;
-  ext_list_access_test_obj =
-      Dart_NewExternalTypedData(Dart_TypedData_kUint8, data, data_length);
-  EXPECT_VALID(ext_list_access_test_obj);
-  TestDirectAccess(lib, ext_list_access_test_obj, Dart_TypedData_kUint8, true);
+  for (intptr_t i = 0; i < 10; i++) {
+    // Test with an regular typed data object.
+    Dart_Handle list_access_test_obj;
+    list_access_test_obj = Dart_Invoke(lib, NewString("main"), 0, NULL);
+    EXPECT_VALID(list_access_test_obj);
+    TestDirectAccess(lib, list_access_test_obj, Dart_TypedData_kInt8, false);
+
+    // Test with an external typed data object.
+    uint8_t data[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+    intptr_t data_length = ARRAY_SIZE(data);
+    Dart_Handle ext_list_access_test_obj;
+    ext_list_access_test_obj =
+        Dart_NewExternalTypedData(Dart_TypedData_kUint8, data, data_length);
+    EXPECT_VALID(ext_list_access_test_obj);
+    TestDirectAccess(lib, ext_list_access_test_obj, Dart_TypedData_kUint8,
+                     true);
+  }
+
+  {
+    MonitorLocker ml(&monitor);
+    while (!done) {
+      ml.Wait();
+    }
+  }
 }
 
 TEST_CASE(DartAPI_TypedDataDirectAccess1Unverified) {

runtime/vm/heap/safepoint.cc

@@ -113,8 +113,14 @@ void SafepointHandler::SafepointThreads(Thread* T) {
         if (FLAG_trace_safepoint && num_attempts > 10) {
           // We have been waiting too long, start logging this as we might
           // have an issue where a thread is not checking in for a safepoint.
-          OS::PrintErr("Attempt:%" Pd " waiting for %d threads to check in\n",
-                       num_attempts, number_threads_not_at_safepoint_);
+          for (Thread* current = isolate()->thread_registry()->active_list();
+               current != NULL; current = current->next()) {
+            if (!current->IsAtSafepoint()) {
+              OS::PrintErr("Attempt:%" Pd
+                           " waiting for thread %s to check in\n",
+                           num_attempts, current->os_thread()->name());
+            }
+          }
         }
       }
     }

runtime/vm/heap/safepoint.h

@@ -290,7 +290,9 @@ class TransitionNativeToVM : public TransitionSafepointState {
   explicit TransitionNativeToVM(Thread* T) : TransitionSafepointState(T) {
     // We are about to execute vm code and so we are not at a safepoint anymore.
     ASSERT(T->execution_state() == Thread::kThreadInNative);
-    T->ExitSafepoint();
+    if (T->no_callback_scope_depth() == 0) {
+      T->ExitSafepoint();
+    }
     T->set_execution_state(Thread::kThreadInVM);
   }
 
@@ -298,7 +300,9 @@ class TransitionNativeToVM : public TransitionSafepointState {
     // We are returning to native code and so we are at a safepoint.
     ASSERT(thread()->execution_state() == Thread::kThreadInVM);
     thread()->set_execution_state(Thread::kThreadInNative);
-    thread()->EnterSafepoint();
+    if (thread()->no_callback_scope_depth() == 0) {
+      thread()->EnterSafepoint();
+    }
   }
 
  private:

runtime/vm/isolate.cc

@@ -1756,11 +1756,6 @@ void Isolate::LowLevelShutdown() {
     }
   }
 
-#if !defined(PRODUCT)
-  // Clean up debugger resources.
-  debugger()->Shutdown();
-#endif
-
   // Close all the ports owned by this isolate.
   PortMap::ClosePorts(message_handler());
 
@@ -1859,6 +1854,9 @@ void Isolate::Shutdown() {
     HandleScope handle_scope(thread);
     ServiceIsolate::SendIsolateShutdownMessage();
     KernelIsolate::NotifyAboutIsolateShutdown(this);
+#if !defined(PRODUCT)
+    debugger()->Shutdown();
+#endif
   }
 
   if (heap_ != nullptr) {
@@ -2552,7 +2550,7 @@ void Isolate::PauseEventHandler() {
     pause_loop_monitor_ = new Monitor();
   }
   Dart_EnterScope();
-  MonitorLocker ml(pause_loop_monitor_);
+  MonitorLocker ml(pause_loop_monitor_, false);
 
   Dart_MessageNotifyCallback saved_notify_callback = message_notify_callback();
   set_message_notify_callback(Isolate::WakePauseEventHandler);

runtime/vm/kernel_loader.cc

@@ -2131,16 +2131,16 @@ RawLibrary* KernelLoader::LookupLibraryOrNull(NameIndex library) {
   RawLibrary* result;
   name_index_handle_ = Smi::New(library);
   {
-    NoSafepointScope no_safepoint_scope(thread_);
     result = kernel_program_info_.LookupLibrary(thread_, name_index_handle_);
+    NoSafepointScope no_safepoint_scope(thread_);
     if (result != Library::null()) {
       return result;
     }
   }
   const String& url = H.DartString(H.CanonicalNameString(library));
   {
-    NoSafepointScope no_safepoint_scope(thread_);
     result = Library::LookupLibrary(thread_, url);
+    NoSafepointScope no_safepoint_scope(thread_);
     if (result == Library::null()) {
       return result;
     }
@@ -2154,9 +2154,9 @@ RawLibrary* KernelLoader::LookupLibraryOrNull(NameIndex library) {
 RawLibrary* KernelLoader::LookupLibrary(NameIndex library) {
   name_index_handle_ = Smi::New(library);
   {
-    NoSafepointScope no_safepoint_scope(thread_);
     RawLibrary* result =
         kernel_program_info_.LookupLibrary(thread_, name_index_handle_);
+    NoSafepointScope no_safepoint_scope(thread_);
     if (result != Library::null()) {
       return result;
     }
@@ -2192,9 +2192,9 @@ RawLibrary* KernelLoader::LookupLibraryFromClass(NameIndex klass) {
 RawClass* KernelLoader::LookupClass(const Library& library, NameIndex klass) {
   name_index_handle_ = Smi::New(klass);
   {
-    NoSafepointScope no_safepoint_scope(thread_);
     RawClass* raw_class =
         kernel_program_info_.LookupClass(thread_, name_index_handle_);
+    NoSafepointScope no_safepoint_scope(thread_);
     if (raw_class != Class::null()) {
       return raw_class;
     }

runtime/vm/lockers.cc

@@ -12,6 +12,11 @@ Monitor::WaitResult MonitorLocker::WaitWithSafepointCheck(Thread* thread,
                                                           int64_t millis) {
   ASSERT(thread == Thread::Current());
   ASSERT(thread->execution_state() == Thread::kThreadInVM);
+#if defined(DEBUG)
+  if (no_safepoint_scope_) {
+    thread->DecrementNoSafepointScopeDepth();
+  }
+#endif
   thread->set_execution_state(Thread::kThreadInBlockedState);
   thread->EnterSafepoint();
   Monitor::WaitResult result = monitor_->Wait(millis);
@@ -26,6 +31,11 @@ Monitor::WaitResult MonitorLocker::WaitWithSafepointCheck(Thread* thread,
     monitor_->Enter();
   }
   thread->set_execution_state(Thread::kThreadInVM);
+#if defined(DEBUG)
+  if (no_safepoint_scope_) {
+    thread->IncrementNoSafepointScopeDepth();
+  }
+#endif
   return result;
 }
 

runtime/vm/object.cc

@@ -2139,6 +2139,7 @@ RawObject* Object::Allocate(intptr_t cls_id, intptr_t size, Heap::Space space) {
   ASSERT(Utils::IsAligned(size, kObjectAlignment));
   Thread* thread = Thread::Current();
   ASSERT(thread->execution_state() == Thread::kThreadInVM);
+  ASSERT(thread->no_safepoint_scope_depth() == 0);
   ASSERT(thread->no_callback_scope_depth() == 0);
   Heap* heap = thread->heap();
 

runtime/vm/service.cc

@@ -4,6 +4,9 @@
 
 #include "vm/service.h"
 
+#include <memory>
+#include <utility>
+
 #include "include/dart_api.h"
 #include "include/dart_native_api.h"
 #include "platform/globals.h"
@@ -13,6 +16,7 @@
 #include "vm/compiler/jit/compiler.h"
 #include "vm/cpu.h"
 #include "vm/dart_api_impl.h"
+#include "vm/dart_api_message.h"
 #include "vm/dart_api_state.h"
 #include "vm/dart_entry.h"
 #include "vm/debugger.h"
@@ -1164,14 +1168,11 @@ void Service::PostEvent(Isolate* isolate,
   json_cobj.value.as_string = const_cast<char*>(event->ToCString());
   list_values[1] = &json_cobj;
 
-  // In certain cases (e.g. in the implementation of Dart_IsolateMakeRunnable)
-  // we do not have a current isolate/thread.
-  auto thread = Thread::Current();
-  if (thread != nullptr) {
-    TransitionVMToNative transition(thread);
-    Dart_PostCObject(ServiceIsolate::Port(), &list_cobj);
-  } else {
-    Dart_PostCObject(ServiceIsolate::Port(), &list_cobj);
+  ApiMessageWriter writer;
+  std::unique_ptr<Message> msg = writer.WriteCMessage(
+      &list_cobj, ServiceIsolate::Port(), Message::kNormalPriority);
+  if (msg != nullptr) {
+    PortMap::PostMessage(std::move(msg));
   }
 }
 

runtime/vm/thread.h

@@ -736,6 +736,7 @@ class Thread : public ThreadState {
   }
 
   void EnterSafepoint() {
+    ASSERT(no_safepoint_scope_depth() == 0);
     // First try a fast update of the thread state to indicate it is at a
     // safepoint.
     if (!TryEnterSafepoint()) {
@@ -765,6 +766,7 @@ class Thread : public ThreadState {
   }
 
   void CheckForSafepoint() {
+    ASSERT(no_safepoint_scope_depth() == 0);
     if (IsSafepointRequested()) {
       BlockForSafepoint();
     }