From 54b197ccc23ab32ac61658b19d1935ef8a5bbf11 Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Wed, 19 Jun 2019 21:31:31 +0000 Subject: [PATCH] [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 Reviewed-by: Alexander Markov --- .../frontend/kernel_translation_helper.cc | 10 +-- runtime/vm/dart_api_impl.cc | 27 +++---- runtime/vm/dart_api_impl_test.cc | 80 ++++++++++++++----- runtime/vm/heap/safepoint.cc | 10 ++- runtime/vm/heap/safepoint.h | 8 +- runtime/vm/isolate.cc | 10 +-- runtime/vm/kernel_loader.cc | 8 +- runtime/vm/lockers.cc | 10 +++ runtime/vm/object.cc | 1 + runtime/vm/service.cc | 17 ++-- runtime/vm/thread.h | 2 + 11 files changed, 118 insertions(+), 65 deletions(-) diff --git a/runtime/vm/compiler/frontend/kernel_translation_helper.cc b/runtime/vm/compiler/frontend/kernel_translation_helper.cc index b999e217778..bd50a0f7db8 100644 --- a/runtime/vm/compiler/frontend/kernel_translation_helper.cc +++ b/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; } diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index 4ebdac973ee..ee311943e99 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/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(); diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc index e500239e452..4841f0debaf 100644 --- a/runtime/vm/dart_api_impl_test.cc +++ b/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(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 3888cdfd02b..d3e3771aa3c 100644 --- a/runtime/vm/heap/safepoint.cc +++ b/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()); + } + } } } } diff --git a/runtime/vm/heap/safepoint.h b/runtime/vm/heap/safepoint.h index 906933ad284..4f3cfb8676d 100644 --- a/runtime/vm/heap/safepoint.h +++ b/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: diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index fe394e4bede..11e65e95807 100644 --- a/runtime/vm/isolate.cc +++ b/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); diff --git a/runtime/vm/kernel_loader.cc b/runtime/vm/kernel_loader.cc index b5001d12c5d..4f60e0f71e2 100644 --- a/runtime/vm/kernel_loader.cc +++ b/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; } diff --git a/runtime/vm/lockers.cc b/runtime/vm/lockers.cc index 7a43e4a2c28..79f6f259460 100644 --- a/runtime/vm/lockers.cc +++ b/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; } diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index ad74bbe930c..1ff3a454564 100644 --- a/runtime/vm/object.cc +++ b/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(); diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc index ed4dcc2cdc5..80ba87fa4e3 100644 --- a/runtime/vm/service.cc +++ b/runtime/vm/service.cc @@ -4,6 +4,9 @@ #include "vm/service.h" +#include +#include + #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(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 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 ab8071e5834..b13b1490b49 100644 --- a/runtime/vm/thread.h +++ b/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(); }