From e211bd28c1cbe25c69d28d2eda306140f2c96ae4 Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Mon, 2 Mar 2020 22:56:06 +0000 Subject: [PATCH] [vm, gc] Set the current isolate to the group's only member before running finalizers. Some embedder code relies on the presence of a current isolate equal to the isolate that created the handle. This code will be need to be updated to be isolate-group aware before we can run finalizers only in the context of the isolate group instead of an isolate. Bug: https://github.com/dart-lang/sdk/issues/40836 Change-Id: I38956c556df551e48ee7e4d0822aa5def8dc4fc5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138013 Commit-Queue: Ryan Macnak Reviewed-by: Siva Annamalai --- runtime/vm/dart_api_impl_test.cc | 44 ++++++++++++++++++++++++++++++++ runtime/vm/heap/heap.cc | 24 ++++++++++++++--- runtime/vm/heap/heap.h | 14 ++++++++++ runtime/vm/heap/marker.cc | 3 +++ runtime/vm/heap/scavenger.cc | 1 + runtime/vm/isolate.h | 1 + runtime/vm/thread.h | 1 + 7 files changed, 84 insertions(+), 4 deletions(-) diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc index 60ce652412d..0bcdb741878 100644 --- a/runtime/vm/dart_api_impl_test.cc +++ b/runtime/vm/dart_api_impl_test.cc @@ -3468,6 +3468,50 @@ TEST_CASE(DartAPI_WeakPersistentHandleExternalAllocationSizeOddReferents) { } } +static void AssertingFinalizer(void* isolate_callback_data, + Dart_WeakPersistentHandle handle, + void* peer) { + Dart_Isolate expected_isolate = reinterpret_cast(peer); + EXPECT_EQ(expected_isolate, Dart_CurrentIsolate()); +} + +// TODO(https://github.com/dart-lang/sdk/issues/40836): Remove. +TEST_CASE(DartAPI_WeakPersistentHandleFinalizerCurrentIsolate) { + { + Dart_EnterScope(); + + Dart_Handle obj = AllocateNewString("ABC"); + void* peer = Dart_CurrentIsolate(); + EXPECT_NOTNULL(peer); + intptr_t external_size = 0; + Dart_NewWeakPersistentHandle(obj, peer, external_size, AssertingFinalizer); + + Dart_ExitScope(); + } + + { + TransitionNativeToVM transition(thread); + GCTestHelper::CollectAllGarbage(); + } + + { + Dart_EnterScope(); + + Dart_Handle obj = AllocateOldString("ABC"); + void* peer = Dart_CurrentIsolate(); + EXPECT_NOTNULL(peer); + intptr_t external_size = 0; + Dart_NewWeakPersistentHandle(obj, peer, external_size, AssertingFinalizer); + + Dart_ExitScope(); + } + + { + TransitionNativeToVM transition(thread); + GCTestHelper::CollectAllGarbage(); + } +} + static Dart_WeakPersistentHandle weak1 = NULL; static Dart_WeakPersistentHandle weak2 = NULL; static Dart_WeakPersistentHandle weak3 = NULL; diff --git a/runtime/vm/heap/heap.cc b/runtime/vm/heap/heap.cc index 2bd5b419a95..a04108bc3d9 100644 --- a/runtime/vm/heap/heap.cc +++ b/runtime/vm/heap/heap.cc @@ -52,6 +52,24 @@ class NoActiveIsolateScope { Isolate* saved_isolate_; }; +RunFinalizersScope::RunFinalizersScope(Thread* thread) : thread_(thread) { + if (!FLAG_enable_isolate_groups) { + ASSERT(thread->IsAtSafepoint() || + (thread->task_kind() == Thread::kMarkerTask)); + IsolateGroup* isolate_group = thread->isolate_group(); + Isolate* isolate = isolate_group->isolates_.First(); + ASSERT(isolate == isolate_group->isolates_.Last()); + saved_isolate_ = thread->isolate_; + thread->isolate_ = isolate; + } +} + +RunFinalizersScope::~RunFinalizersScope() { + if (!FLAG_enable_isolate_groups) { + thread_->isolate_ = saved_isolate_; + } +} + Heap::Heap(IsolateGroup* isolate_group, intptr_t max_new_gen_semi_words, intptr_t max_old_gen_words) @@ -460,8 +478,7 @@ void Heap::EvacuateNewSpace(Thread* thread, GCReason reason) { } void Heap::CollectNewSpaceGarbage(Thread* thread, GCReason reason) { - // TODO(40836) : Uncomment this line once the issue is fixed. - // NoActiveIsolateScope no_active_isolate_scope; + NoActiveIsolateScope no_active_isolate_scope; ASSERT((reason != kOldSpace) && (reason != kPromotion)); if (thread->isolate_group() == Dart::vm_isolate()->group()) { // The vm isolate cannot safely collect garbage due to unvisited read-only @@ -495,8 +512,7 @@ void Heap::CollectNewSpaceGarbage(Thread* thread, GCReason reason) { void Heap::CollectOldSpaceGarbage(Thread* thread, GCType type, GCReason reason) { - // TODO(40836) : Uncomment this line once the issue is fixed. - // NoActiveIsolateScope no_active_isolate_scope; + NoActiveIsolateScope no_active_isolate_scope; ASSERT(reason != kNewSpace); ASSERT(type != kScavenge); diff --git a/runtime/vm/heap/heap.h b/runtime/vm/heap/heap.h index 34ab784564f..f72472ef9cb 100644 --- a/runtime/vm/heap/heap.h +++ b/runtime/vm/heap/heap.h @@ -30,6 +30,20 @@ class ServiceEvent; class TimelineEventScope; class VirtualMemory; +// Temporarily enter the only isolate in the group before running weak handle +// finalizers to keep existing embedder code working. Remove once embedders +// are updated to think in terms of isolate groups. +// TODO(https://github.com/dart-lang/sdk/issues/40836): Remove. +class RunFinalizersScope { + public: + explicit RunFinalizersScope(Thread* thread); + ~RunFinalizersScope(); + + private: + Thread* thread_; + Isolate* saved_isolate_; +}; + class Heap { public: enum Space { diff --git a/runtime/vm/heap/marker.cc b/runtime/vm/heap/marker.cc index d35329637f0..75bc357953c 100644 --- a/runtime/vm/heap/marker.cc +++ b/runtime/vm/heap/marker.cc @@ -465,9 +465,12 @@ void GCMarker::IterateWeakRoots(Thread* thread) { void GCMarker::ProcessWeakHandles(Thread* thread) { TIMELINE_FUNCTION_GC_DURATION(thread, "ProcessWeakHandles"); + MarkingWeakVisitor visitor(thread); ApiState* state = isolate_group_->api_state(); ASSERT(state != NULL); + + RunFinalizersScope tcis(thread); isolate_group_->VisitWeakPersistentHandles(&visitor); } diff --git a/runtime/vm/heap/scavenger.cc b/runtime/vm/heap/scavenger.cc index d3c20b2865a..f1d4b2a57c5 100644 --- a/runtime/vm/heap/scavenger.cc +++ b/runtime/vm/heap/scavenger.cc @@ -772,6 +772,7 @@ bool Scavenger::IsUnreachable(RawObject** p) { void Scavenger::IterateWeakRoots(IsolateGroup* isolate_group, HandleVisitor* visitor) { + RunFinalizersScope tcis(Thread::Current()); isolate_group->VisitWeakPersistentHandles(visitor); } diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index 0bc9b3a1066..f2389bb26c6 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -476,6 +476,7 @@ class IsolateGroup : public IntrusiveDListEntry { private: friend class Heap; friend class StackFrame; // For `[isolates_].First()`. + friend class RunFinalizersScope; // For `[isolates_].First()`. #define ISOLATE_GROUP_FLAG_BITS(V) V(CompactionInProgress) diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h index 8353003bc83..5524c5e6582 100644 --- a/runtime/vm/thread.h +++ b/runtime/vm/thread.h @@ -1034,6 +1034,7 @@ class Thread : public ThreadState { friend Isolate* CreateWithinExistingIsolateGroup(IsolateGroup*, const char*, char**); + friend class RunFinalizersScope; DISALLOW_COPY_AND_ASSIGN(Thread); };