From ab6eb9332fb2db1c76deaaa7c8f265f56f793364 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Fri, 19 Oct 2018 19:35:25 +0000 Subject: [PATCH] [VM] Fix flaky crash when unwinding the mutator stack during GC The mutator thread structure is kept alive until the death of the isolate. Yet the mutator thread does not have to be scheduled all the time. A native call, for example, can suspend the isolate via Dart_ExitIsolate to perform other work. This particular flaky crash had precisely this problem: The mutator thread suspended itself with an IsolateSaver scope (which uses Dart_ExitIsolate) and invoked Dart_NewNativePort. While at the same time the background compiler for that isolate triggered an oldspace allocation, which triggered a marking task. The marker task needs to traverses the mutator stack and hits a frame with a deoptimization marker, which causes it to access the mutator thread's isolate pointer, which was incorrectly NULL. Fixes https://github.com/dart-lang/sdk/issues/34748 Fixes https://github.com/dart-lang/sdk/issues/34323 Change-Id: I80440856f72b3c194a516084ddc254b2e56740d8 Reviewed-on: https://dart-review.googlesource.com/c/80860 Reviewed-by: Vyacheslav Egorov Commit-Queue: Martin Kustermann --- runtime/vm/isolate.cc | 14 ++++++++++- runtime/vm/thread.cc | 46 ++++++++++++++++++++--------------- runtime/vm/thread_registry.cc | 4 +++ 3 files changed, 43 insertions(+), 21 deletions(-) diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 114918a5058bd..e3a60be2187a2 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -2854,7 +2854,19 @@ void Isolate::UnscheduleThread(Thread* thread, scheduled_mutator_thread_->end_ = 0; scheduled_mutator_thread_ = NULL; } - thread->isolate_ = NULL; + // Even if we unschedule the mutator thread, e.g. via calling + // `Dart_ExitIsolate()` inside a native, we might still have one or more Dart + // stacks active, which e.g. GC marker threads want to visit. So we don't + // clear out the isolate pointer if we are on the mutator thread. + // + // The [thread] structure for the mutator thread is kept alive in the thread + // registry even if the mutator thread is temporarily unscheduled. + // + // All other threads are not allowed to unschedule themselves and schedule + // again later on. + if (!is_mutator) { + thread->isolate_ = NULL; + } thread->heap_ = NULL; thread->set_os_thread(NULL); thread->set_execution_state(Thread::kThreadInNative); diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc index 505b787d0ab42..bc6c6ad287083 100644 --- a/runtime/vm/thread.cc +++ b/runtime/vm/thread.cc @@ -688,26 +688,32 @@ void Thread::VisitObjectPointers(ObjectPointerVisitor* visitor, scope = scope->previous(); } - // The MarkTask, which calls this method, can run on a different thread. We - // therefore assume the mutator is at a safepoint and we can iterate it's - // stack. - // TODO(vm-team): It would be beneficial to be able to ask the mutator thread - // whether it is in fact blocked at the moment (at a "safepoint") so we can - // safely iterate it's stack. - // - // Unfortunately we cannot use `this->IsAtSafepoint()` here because that will - // return `false` even though the mutator thread is waiting for mark tasks - // (which iterate it's stack) to finish. - const StackFrameIterator::CrossThreadPolicy cross_thread_policy = - StackFrameIterator::kAllowCrossThreadIteration; - - // Iterate over all the stack frames and visit objects on the stack. - StackFrameIterator frames_iterator(top_exit_frame_info(), validation_policy, - this, cross_thread_policy); - StackFrame* frame = frames_iterator.NextFrame(); - while (frame != NULL) { - frame->VisitObjectPointers(visitor); - frame = frames_iterator.NextFrame(); + // Only the mutator thread can run Dart code. + if (IsMutatorThread()) { + // The MarkTask, which calls this method, can run on a different thread. We + // therefore assume the mutator is at a safepoint and we can iterate it's + // stack. + // TODO(vm-team): It would be beneficial to be able to ask the mutator + // thread whether it is in fact blocked at the moment (at a "safepoint") so + // we can safely iterate it's stack. + // + // Unfortunately we cannot use `this->IsAtSafepoint()` here because that + // will return `false` even though the mutator thread is waiting for mark + // tasks (which iterate it's stack) to finish. + const StackFrameIterator::CrossThreadPolicy cross_thread_policy = + StackFrameIterator::kAllowCrossThreadIteration; + + // Iterate over all the stack frames and visit objects on the stack. + StackFrameIterator frames_iterator(top_exit_frame_info(), validation_policy, + this, cross_thread_policy); + StackFrame* frame = frames_iterator.NextFrame(); + while (frame != NULL) { + frame->VisitObjectPointers(visitor); + frame = frames_iterator.NextFrame(); + } + } else { + // We are not on the mutator thread. + RELEASE_ASSERT(top_exit_frame_info() == 0); } } diff --git a/runtime/vm/thread_registry.cc b/runtime/vm/thread_registry.cc index b1bcb2ce7001f..bb0297339687f 100644 --- a/runtime/vm/thread_registry.cc +++ b/runtime/vm/thread_registry.cc @@ -16,6 +16,10 @@ ThreadRegistry::~ThreadRegistry() { MonitorLocker ml(threads_lock()); // At this point the active list should be empty. ASSERT(active_list_ == NULL); + + // The [mutator_thread_] is kept alive until the destruction of the isolate. + mutator_thread_->isolate_ = nullptr; + // We have cached the mutator thread, delete it. delete mutator_thread_; mutator_thread_ = NULL;