The sweeper must not be running during isolate shutdown.

In release mode, there seems to be nothing to prevent this.
In debug mode, the "Verify" call waits for the sweeper, but there is still a race between the task count update and the ExitIsolateAsHelper call, which could cause problems.

Fix both of these, and add more assertions and verbose error messages.

- make sweeper task cleanly exit isolate *before* notifying
- wait for sweeper before shutting down isolate
- verbose pthread failures

BUG=
R=asiva@google.com

Review URL: https://codereview.chromium.org//1233563004 .

Author: kodandersson

runtime/vm/gc_sweeper.cc

@@ -137,13 +137,14 @@ class SweeperTask : public ThreadPool::Task {
       if (page == last_) break;
       page = next_page;
     }
+    // Exit isolate cleanly *before* notifying it, to avoid shutdown race.
+    Thread::ExitIsolateAsHelper();
     // This sweeper task is done. Notify the original isolate.
     {
       MonitorLocker ml(old_space_->tasks_lock());
       old_space_->set_tasks(old_space_->tasks() - 1);
       ml.Notify();
     }
-    Thread::ExitIsolateAsHelper();
   }
 
  private:

runtime/vm/isolate.cc

@@ -1437,6 +1437,15 @@ void Isolate::Shutdown() {
   // avoid exposing it in a state of decay.
   RemoveIsolateFromList(this);
 
+  if (heap_ != NULL) {
+    // Wait for any concurrent GC tasks to finish before shutting down.
+    PageSpace* old_space = heap_->old_space();
+    MonitorLocker ml(old_space->tasks_lock());
+    while (old_space->tasks() > 0) {
+      ml.Wait();
+    }
+  }
+
   // Create an area where we do have a zone and a handle scope so that we can
   // call VM functions while tearing this isolate down.
   {
@@ -1490,6 +1499,8 @@ void Isolate::Shutdown() {
   // TODO(5411455): For now just make sure there are no current isolates
   // as we are shutting down the isolate.
   Thread::ExitIsolate();
+  // All threads should have exited by now.
+  thread_registry()->CheckNotScheduled(this);
   Profiler::ShutdownProfilingForIsolate(this);
 }
 

runtime/vm/os_thread_android.cc

@@ -23,6 +23,14 @@ namespace dart {
   }
 
 
+#if defined(DEBUG)
+#define ASSERT_PTHREAD_SUCCESS(result) VALIDATE_PTHREAD_RESULT(result)
+#else
+// NOTE: This (currently) expands to a no-op.
+#define ASSERT_PTHREAD_SUCCESS(result) ASSERT(result == 0)
+#endif
+
+
 #ifdef DEBUG
 #define RETURN_ON_PTHREAD_FAILURE(result) \
   if (result != 0) { \
@@ -217,7 +225,7 @@ void Mutex::Lock() {
   int result = pthread_mutex_lock(data_.mutex());
   // Specifically check for dead lock to help debugging.
   ASSERT(result != EDEADLK);
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
   // When running with assertions enabled we do track the owner.
 #if defined(DEBUG)
   owner_ = OSThread::GetCurrentThreadId();
@@ -231,7 +239,7 @@ bool Mutex::TryLock() {
   if (result == EBUSY) {
     return false;
   }
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
   // When running with assertions enabled we do track the owner.
 #if defined(DEBUG)
   owner_ = OSThread::GetCurrentThreadId();
@@ -249,7 +257,7 @@ void Mutex::Unlock() {
   int result = pthread_mutex_unlock(data_.mutex());
   // Specifically check for wrong thread unlocking to aid debugging.
   ASSERT(result != EPERM);
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
 }
 
 

runtime/vm/os_thread_linux.cc

@@ -24,6 +24,14 @@ namespace dart {
   }
 
 
+#if defined(DEBUG)
+#define ASSERT_PTHREAD_SUCCESS(result) VALIDATE_PTHREAD_RESULT(result)
+#else
+// NOTE: This (currently) expands to a no-op.
+#define ASSERT_PTHREAD_SUCCESS(result) ASSERT(result == 0)
+#endif
+
+
 #ifdef DEBUG
 #define RETURN_ON_PTHREAD_FAILURE(result) \
   if (result != 0) { \
@@ -218,7 +226,7 @@ void Mutex::Lock() {
   int result = pthread_mutex_lock(data_.mutex());
   // Specifically check for dead lock to help debugging.
   ASSERT(result != EDEADLK);
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
   // When running with assertions enabled we do track the owner.
 #if defined(DEBUG)
   owner_ = OSThread::GetCurrentThreadId();
@@ -232,7 +240,7 @@ bool Mutex::TryLock() {
   if (result == EBUSY) {
     return false;
   }
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
   // When running with assertions enabled we do track the owner.
 #if defined(DEBUG)
   owner_ = OSThread::GetCurrentThreadId();
@@ -250,7 +258,7 @@ void Mutex::Unlock() {
   int result = pthread_mutex_unlock(data_.mutex());
   // Specifically check for wrong thread unlocking to aid debugging.
   ASSERT(result != EPERM);
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
 }
 
 

runtime/vm/os_thread_macos.cc

@@ -31,6 +31,14 @@ namespace dart {
   }
 
 
+#if defined(DEBUG)
+#define ASSERT_PTHREAD_SUCCESS(result) VALIDATE_PTHREAD_RESULT(result)
+#else
+// NOTE: This (currently) expands to a no-op.
+#define ASSERT_PTHREAD_SUCCESS(result) ASSERT(result == 0)
+#endif
+
+
 #ifdef DEBUG
 #define RETURN_ON_PTHREAD_FAILURE(result) \
   if (result != 0) { \
@@ -223,7 +231,7 @@ void Mutex::Lock() {
   int result = pthread_mutex_lock(data_.mutex());
   // Specifically check for dead lock to help debugging.
   ASSERT(result != EDEADLK);
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
   // When running with assertions enabled we do track the owner.
 #if defined(DEBUG)
   owner_ = OSThread::GetCurrentThreadId();
@@ -237,7 +245,7 @@ bool Mutex::TryLock() {
   if ((result == EBUSY) || (result == EDEADLK)) {
     return false;
   }
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
   // When running with assertions enabled we do track the owner.
 #if defined(DEBUG)
   owner_ = OSThread::GetCurrentThreadId();
@@ -255,7 +263,7 @@ void Mutex::Unlock() {
   int result = pthread_mutex_unlock(data_.mutex());
   // Specifically check for wrong thread unlocking to aid debugging.
   ASSERT(result != EPERM);
-  ASSERT(result == 0);  // Verify no other errors.
+  ASSERT_PTHREAD_SUCCESS(result);  // Verify no other errors.
 }
 
 

runtime/vm/thread.cc

@@ -27,6 +27,12 @@ static void DeleteThread(void* thread) {
 }
 
 
+Thread::~Thread() {
+  // We should cleanly exit any isolate before destruction.
+  ASSERT(isolate_ == NULL);
+}
+
+
 void Thread::InitOnceBeforeIsolate() {
   ASSERT(thread_key_ == OSThread::kUnsetThreadLocalKey);
   thread_key_ = OSThread::CreateThreadLocal(DeleteThread);

runtime/vm/thread.h

@@ -75,6 +75,8 @@ class Thread {
   static void InitOnceBeforeIsolate();
   static void InitOnceAfterObjectAndStubCode();
 
+  ~Thread();
+
   // The topmost zone used for allocation in this thread.
   Zone* zone() const { return state_.zone; }
 

runtime/vm/thread_registry.h

@@ -63,6 +63,19 @@ class ThreadRegistry {
     return (FindEntry(thread) != NULL);
   }
 
+  void CheckNotScheduled(Isolate* isolate) {
+    MutexLocker ml(mutex_);
+    for (int i = 0; i < entries_.length(); ++i) {
+      const Entry& entry = entries_[i];
+      if (entry.scheduled) {
+        FATAL3("Isolate %p still scheduled on %p (whose isolate_ is %p)\n",
+               isolate,
+               entry.thread,
+               entry.thread->isolate());
+      }
+    }
+  }
+
   void VisitObjectPointers(ObjectPointerVisitor* visitor) {
     MutexLocker ml(mutex_);
     for (int i = 0; i < entries_.length(); ++i) {