From afe3c12b25d4bd433145b4f4d10377c3a73485ba Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Thu, 6 Jan 2022 18:38:28 +0000 Subject: [PATCH] [vm] Move handles for the no-callbacks and unwind-in-progress errors to the VM isolate so there is no Dart allocation and no handle allocation during these errors. TEST=ci Change-Id: I0567cf8a9b4361df312cb4958fc7438296b65604 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/226605 Reviewed-by: Siva Annamalai Commit-Queue: Ryan Macnak --- runtime/vm/dart_api_impl.cc | 35 ++++++++++------------ runtime/vm/dart_api_impl.h | 27 ++++++++++++----- runtime/vm/dart_api_impl_test.cc | 4 +-- runtime/vm/dart_api_state.h | 51 +------------------------------- runtime/vm/object.cc | 16 ++++++---- runtime/vm/object.h | 3 +- 6 files changed, 49 insertions(+), 87 deletions(-) diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index a4e6c1a8bc3..8b8468d3fb3 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/runtime/vm/dart_api_impl.cc @@ -95,6 +95,8 @@ Dart_Handle Api::true_handle_ = NULL; Dart_Handle Api::false_handle_ = NULL; Dart_Handle Api::null_handle_ = NULL; Dart_Handle Api::empty_string_handle_ = NULL; +Dart_Handle Api::no_callbacks_error_handle_ = NULL; +Dart_Handle Api::unwind_in_progress_error_handle_ = NULL; const char* CanonicalFunction(const char* func) { if (strncmp(func, "dart::", 6) == 0) { @@ -478,23 +480,6 @@ Dart_Handle Api::NewArgumentError(const char* format, ...) { return Api::NewHandle(T, error.ptr()); } -Dart_Handle Api::AcquiredError(IsolateGroup* isolate_group) { - ApiState* state = isolate_group->api_state(); - ASSERT(state != NULL); - PersistentHandle* acquired_error_handle = state->AcquiredError(); - return reinterpret_cast(acquired_error_handle); -} - -Dart_Handle Api::UnwindInProgressError() { - Thread* T = Thread::Current(); - CHECK_API_SCOPE(T); - TransitionToVM transition(T); - HANDLESCOPE(T); - const String& message = String::Handle( - Z, String::New("No api calls are allowed while unwind is in progress")); - return Api::NewHandle(T, UnwindError::New(message)); -} - bool Api::IsValid(Dart_Handle handle) { Isolate* isolate = Isolate::Current(); Thread* thread = Thread::Current(); @@ -551,6 +536,14 @@ void Api::InitHandles() { ASSERT(empty_string_handle_ == NULL); empty_string_handle_ = InitNewReadOnlyApiHandle(Symbols::Empty().ptr()); + + ASSERT(no_callbacks_error_handle_ == NULL); + no_callbacks_error_handle_ = + InitNewReadOnlyApiHandle(Object::no_callbacks_error().ptr()); + + ASSERT(unwind_in_progress_error_handle_ == NULL); + unwind_in_progress_error_handle_ = + InitNewReadOnlyApiHandle(Object::unwind_in_progress_error().ptr()); } void Api::Cleanup() { @@ -558,6 +551,8 @@ void Api::Cleanup() { false_handle_ = NULL; null_handle_ = NULL; empty_string_handle_ = NULL; + no_callbacks_error_handle_ = NULL; + unwind_in_progress_error_handle_ = NULL; } bool Api::StringGetPeerHelper(NativeArguments* arguments, @@ -1156,9 +1151,9 @@ DART_EXPORT void Dart_DeletePersistentHandle(Dart_PersistentHandle object) { ApiState* state = isolate_group->api_state(); ASSERT(state != NULL); ASSERT(state->IsActivePersistentHandle(object)); - PersistentHandle* ref = PersistentHandle::Cast(object); - ASSERT(!state->IsProtectedHandle(ref)); - if (!state->IsProtectedHandle(ref)) { + ASSERT(!Api::IsProtectedHandle(object)); + if (!Api::IsProtectedHandle(object)) { + PersistentHandle* ref = PersistentHandle::Cast(object); state->FreePersistentHandle(ref); } } diff --git a/runtime/vm/dart_api_impl.h b/runtime/vm/dart_api_impl.h index 47f9b7a08df..73f681fac9c 100644 --- a/runtime/vm/dart_api_impl.h +++ b/runtime/vm/dart_api_impl.h @@ -181,12 +181,6 @@ class Api : AllStatic { // Gets the handle used to designate successful return. static Dart_Handle Success() { return Api::True(); } - // Gets the handle which holds the pre-created acquired error object. - static Dart_Handle AcquiredError(IsolateGroup* isolate_group); - - // Gets the handle for unwind-is-in-progress error. - static Dart_Handle UnwindInProgressError(); - // Returns true if the handle holds a Smi. static bool IsSmi(Dart_Handle handle) { // Important: we do not require current thread to be in VM state because @@ -241,6 +235,22 @@ class Api : AllStatic { // Gets a handle to EmptyString. static Dart_Handle EmptyString() { return empty_string_handle_; } + // Gets the handle which holds the pre-created acquired error object. + static Dart_Handle NoCallbacksError() { return no_callbacks_error_handle_; } + + // Gets the handle for unwind-is-in-progress error. + static Dart_Handle UnwindInProgressError() { + return unwind_in_progress_error_handle_; + } + + static bool IsProtectedHandle(Dart_Handle object) { + if (object == NULL) return false; + return (object == true_handle_) || (object == false_handle_) || + (object == null_handle_) || (object == empty_string_handle_) || + (object == no_callbacks_error_handle_) || + (object == unwind_in_progress_error_handle_); + } + // Retrieves the top ApiLocalScope. static ApiLocalScope* TopScope(Thread* thread); @@ -314,6 +324,8 @@ class Api : AllStatic { static Dart_Handle false_handle_; static Dart_Handle null_handle_; static Dart_Handle empty_string_handle_; + static Dart_Handle no_callbacks_error_handle_; + static Dart_Handle unwind_in_progress_error_handle_; friend class ApiNativeScope; }; @@ -326,8 +338,7 @@ class Api : AllStatic { #define CHECK_CALLBACK_STATE(thread) \ if (thread->no_callback_scope_depth() != 0) { \ - return reinterpret_cast( \ - Api::AcquiredError(thread->isolate_group())); \ + return reinterpret_cast(Api::NoCallbacksError()); \ } \ if (thread->is_unwind_in_progress()) { \ return reinterpret_cast(Api::UnwindInProgressError()); \ diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc index 02cbfa3e725..c9851a666ad 100644 --- a/runtime/vm/dart_api_impl_test.cc +++ b/runtime/vm/dart_api_impl_test.cc @@ -2602,9 +2602,7 @@ static void TestDirectAccess(Dart_Handle lib, // 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."); + EXPECT_ERROR(result, "Callbacks into the Dart VM are currently prohibited"); // Now modify the values in the directly accessible array and then check // it we see the changes back in dart. diff --git a/runtime/vm/dart_api_state.h b/runtime/vm/dart_api_state.h index e23252efee2..f3f97755a4d 100644 --- a/runtime/vm/dart_api_state.h +++ b/runtime/vm/dart_api_state.h @@ -702,31 +702,7 @@ class ApiGrowableArray : public BaseGrowableArray { // group basis and destroyed when the isolate group is shutdown. class ApiState { public: - ApiState() - : persistent_handles_(), - weak_persistent_handles_(), - null_(NULL), - true_(NULL), - false_(NULL), - acquired_error_(NULL) {} - ~ApiState() { - if (null_ != NULL) { - persistent_handles_.FreeHandle(null_); - null_ = NULL; - } - if (true_ != NULL) { - persistent_handles_.FreeHandle(true_); - true_ = NULL; - } - if (false_ != NULL) { - persistent_handles_.FreeHandle(false_); - false_ = NULL; - } - if (acquired_error_ != NULL) { - persistent_handles_.FreeHandle(acquired_error_); - acquired_error_ = NULL; - } - } + ApiState() : persistent_handles_(), weak_persistent_handles_() {} void MergeOtherApiState(ApiState* api_state); @@ -790,30 +766,11 @@ class ApiState { !weak_persistent_handles_.IsFreeHandle(object); } - bool IsProtectedHandle(PersistentHandle* object) { - MutexLocker ml(&mutex_); - if (object == NULL) return false; - return object == null_ || object == true_ || object == false_; - } - int CountPersistentHandles() { MutexLocker ml(&mutex_); return persistent_handles_.CountHandles(); } - PersistentHandle* AcquiredError() { - // The ApiError pre-allocated in the "vm-isolate" since we will not be able - // to allocate it when the error actually occurs. - // When the error occurs there will be outstanding acquires to internal - // data pointers making it unsafe to allocate objects on the dart heap. - MutexLocker ml(&mutex_); - if (acquired_error_ == nullptr) { - acquired_error_ = persistent_handles_.AllocateHandle(); - acquired_error_->set_ptr(ApiError::typed_data_acquire_error()); - } - return acquired_error_; - } - void RunWithLockedPersistentHandles( std::function fun) { MutexLocker ml(&mutex_); @@ -835,12 +792,6 @@ class ApiState { FinalizablePersistentHandles weak_persistent_handles_; WeakTable acquired_table_; - // Persistent handles to important objects. - PersistentHandle* null_; - PersistentHandle* true_; - PersistentHandle* false_; - PersistentHandle* acquired_error_; - DISALLOW_COPY_AND_ASSIGN(ApiState); }; diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 77151f02ee2..cb4dcdfc016 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -1158,10 +1158,14 @@ void Object::Init(IsolateGroup* isolate_group) { String& error_str = String::Handle(); error_str = String::New( - "Internal Dart data pointers have been acquired, please release them " - "using Dart_TypedDataReleaseData.", + "Callbacks into the Dart VM are currently prohibited. Either there are " + "outstanding pointers from Dart_TypedDataAcquireData that have not been " + "released with Dart_TypedDataReleaseData, or a finalizer is running.", Heap::kOld); - *typed_data_acquire_error_ = ApiError::New(error_str, Heap::kOld); + *no_callbacks_error_ = ApiError::New(error_str, Heap::kOld); + error_str = String::New( + "No api calls are allowed while unwind is in progress", Heap::kOld); + *unwind_in_progress_error_ = UnwindError::New(error_str, Heap::kOld); error_str = String::New("SnapshotWriter Error", Heap::kOld); *snapshot_writer_error_ = LanguageError::New(error_str, Report::kError, Heap::kOld); @@ -1238,8 +1242,10 @@ void Object::Init(IsolateGroup* isolate_group) { ASSERT(bool_false_->IsBool()); ASSERT(smi_illegal_cid_->IsSmi()); ASSERT(smi_zero_->IsSmi()); - ASSERT(!typed_data_acquire_error_->IsSmi()); - ASSERT(typed_data_acquire_error_->IsApiError()); + ASSERT(!no_callbacks_error_->IsSmi()); + ASSERT(no_callbacks_error_->IsApiError()); + ASSERT(!unwind_in_progress_error_->IsSmi()); + ASSERT(unwind_in_progress_error_->IsUnwindError()); ASSERT(!snapshot_writer_error_->IsSmi()); ASSERT(snapshot_writer_error_->IsLanguageError()); ASSERT(!branch_offset_error_->IsSmi()); diff --git a/runtime/vm/object.h b/runtime/vm/object.h index c402aab8e2f..ab78804ffc9 100644 --- a/runtime/vm/object.h +++ b/runtime/vm/object.h @@ -453,7 +453,8 @@ class Object { V(Bool, bool_false) \ V(Smi, smi_illegal_cid) \ V(Smi, smi_zero) \ - V(ApiError, typed_data_acquire_error) \ + V(ApiError, no_callbacks_error) \ + V(UnwindError, unwind_in_progress_error) \ V(LanguageError, snapshot_writer_error) \ V(LanguageError, branch_offset_error) \ V(LanguageError, speculative_inlining_error) \