From 54b730b3df58b4487e18b32f2febb61cef78d187 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Fri, 28 Feb 2020 15:43:51 +0000 Subject: [PATCH] [vm/concurrency] Ensure CreateWithinExistingIsolateGroup is re-setting the cached isolate_group pointer in the mutator thread When moving the newly spawned isolate from the spawnee group to the main group we have to ensure to update the cached `Isolate::mutator_thread_->isolate_group_` pointer (which we do not reset when descheduling mutator threads (for other threads we do)). This will fix the ASAN crash in this test: tools/test.py -n dartk-asan-linux-release-x64 lib_2/isolate/mandel_isolate_test Fixes https://github.com/dart-lang/sdk/issues/40756 Change-Id: I5a897098106e6748cf4e58bd5d2799ce28b130a8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/136971 Reviewed-by: Alexander Aprelev Commit-Queue: Martin Kustermann --- runtime/vm/dart_api_impl.cc | 10 ++++++++++ runtime/vm/isolate.cc | 8 +++++++- runtime/vm/thread.h | 3 +++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index e83dc2fb2e7..78f1f6b77fd 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/runtime/vm/dart_api_impl.cc @@ -1319,6 +1319,16 @@ Isolate* CreateWithinExistingIsolateGroup(IsolateGroup* group, group->RegisterIsolateLocked(isolate); isolate->class_table()->shared_class_table_ = group->class_table(); + // Even though the mutator thread was descheduled, it will still + // retain its [Thread] structure with valid isolate/isolate_group + // pointers. + // If GC happens before the mutator gets scheduled again, we have to + // ensure the isolate group change is reflected in the threads + // structure. + ASSERT(isolate->mutator_thread() != nullptr); + ASSERT(isolate->mutator_thread()->isolate_group() == spawning_group); + isolate->mutator_thread()->isolate_group_ = group; + // Allow other old space GC tasks to run again. { auto old_space = group->heap()->old_space(); diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 86efdb332ff..4b3e5e5ee3c 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -433,7 +433,8 @@ void IsolateGroup::UnscheduleThreadLocked(MonitorLocker* ml, // All other threads are not allowed to unschedule themselves and schedule // again later on. if (!is_mutator) { - thread->isolate_ = nullptr; + ASSERT(thread->isolate_ == nullptr); + thread->isolate_group_ = nullptr; } thread->heap_ = nullptr; thread->set_os_thread(nullptr); @@ -3415,6 +3416,11 @@ void Isolate::UnscheduleThread(Thread* thread, ASSERT(mutator_thread_ == thread); ASSERT(mutator_thread_ == scheduled_mutator_thread_); scheduled_mutator_thread_ = nullptr; + } else { + // We only reset the isolate pointer for non-mutator threads, since mutator + // threads can still be visited during GC even if unscheduled. + // See also IsolateGroup::UnscheduleThreadLocked` + thread->isolate_ = nullptr; } thread->field_table_values_ = nullptr; group()->UnscheduleThreadLocked(&ml, thread, is_mutator, bypass_safepoint); diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h index 64485ce0afa..8353003bc83 100644 --- a/runtime/vm/thread.h +++ b/runtime/vm/thread.h @@ -1031,6 +1031,9 @@ class Thread : public ThreadState { friend class CompilerState; friend class compiler::target::Thread; friend class FieldTable; + friend Isolate* CreateWithinExistingIsolateGroup(IsolateGroup*, + const char*, + char**); DISALLOW_COPY_AND_ASSIGN(Thread); };