[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>
diff --git a/runtime/vm/compiler/frontend/kernel_translation_helper.cc b/runtime/vm/compiler/frontend/kernel_translation_helper.cc
index b999e21..bd50a0f 100644
--- a/runtime/vm/compiler/frontend/kernel_translation_helper.cc
+++ b/runtime/vm/compiler/frontend/kernel_translation_helper.cc
@@ -499,10 +499,9 @@
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 @@
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;
}
diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc
index 4ebdac9..ee31194 100644
--- a/runtime/vm/dart_api_impl.cc
+++ b/runtime/vm/dart_api_impl.cc
@@ -1465,13 +1465,14 @@
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 @@
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 @@
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 @@
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 @@
!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();
diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc
index e500239..4841f0d 100644
--- a/runtime/vm/dart_api_impl_test.cc
+++ b/runtime/vm/dart_api_impl_test.cc
@@ -2337,6 +2337,7 @@
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 @@
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 @@
}
// Release direct access to the typed data object.
+ EXPECT(!Thread::Current()->IsAtSafepoint());
result = Dart_TypedDataReleaseData(array);
EXPECT_VALID(result);
@@ -2369,6 +2369,29 @@
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 @@
// 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) {
diff --git a/runtime/vm/heap/safepoint.cc b/runtime/vm/heap/safepoint.cc
index 3888cdf..d3e3771 100644
--- a/runtime/vm/heap/safepoint.cc
+++ b/runtime/vm/heap/safepoint.cc
@@ -113,8 +113,14 @@
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());
+ }
+ }
}
}
}
diff --git a/runtime/vm/heap/safepoint.h b/runtime/vm/heap/safepoint.h
index 906933a..4f3cfb8 100644
--- a/runtime/vm/heap/safepoint.h
+++ b/runtime/vm/heap/safepoint.h
@@ -290,7 +290,9 @@
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 @@
// 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:
diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
index fe394e4..11e65e9 100644
--- a/runtime/vm/isolate.cc
+++ b/runtime/vm/isolate.cc
@@ -1756,11 +1756,6 @@
}
}
-#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 @@
HandleScope handle_scope(thread);
ServiceIsolate::SendIsolateShutdownMessage();
KernelIsolate::NotifyAboutIsolateShutdown(this);
+#if !defined(PRODUCT)
+ debugger()->Shutdown();
+#endif
}
if (heap_ != nullptr) {
@@ -2552,7 +2550,7 @@
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);
diff --git a/runtime/vm/kernel_loader.cc b/runtime/vm/kernel_loader.cc
index b5001d1..4f60e0f 100644
--- a/runtime/vm/kernel_loader.cc
+++ b/runtime/vm/kernel_loader.cc
@@ -2131,16 +2131,16 @@
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::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 @@
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;
}
diff --git a/runtime/vm/lockers.cc b/runtime/vm/lockers.cc
index 7a43e4a..79f6f25 100644
--- a/runtime/vm/lockers.cc
+++ b/runtime/vm/lockers.cc
@@ -12,6 +12,11 @@
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_->Enter();
}
thread->set_execution_state(Thread::kThreadInVM);
+#if defined(DEBUG)
+ if (no_safepoint_scope_) {
+ thread->IncrementNoSafepointScopeDepth();
+ }
+#endif
return result;
}
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index ad74bbe..1ff3a45 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -2139,6 +2139,7 @@
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();
diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc
index ed4dcc2..80ba87f 100644
--- a/runtime/vm/service.cc
+++ b/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 @@
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));
}
}
diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h
index ab8071e..b13b149 100644
--- a/runtime/vm/thread.h
+++ b/runtime/vm/thread.h
@@ -736,6 +736,7 @@
}
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 @@
}
void CheckForSafepoint() {
+ ASSERT(no_safepoint_scope_depth() == 0);
if (IsSafepointRequested()) {
BlockForSafepoint();
}
