[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 dart-lang/sdk#34748
Fixes dart-lang/sdk#34323

Change-Id: I80440856f72b3c194a516084ddc254b2e56740d8
Reviewed-on: https://dart-review.googlesource.com/c/80860
Reviewed-by: Vyacheslav Egorov <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>

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);

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);
   }
 }
 

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;