d8: Fix some TSAN bugs

src/d8.cc

Index: src/d8.cc
diff --git a/src/d8.cc b/src/d8.cc
index 0b737c6ffe291e6913269f0618d4608127fe5210..ecf790ff68c66d76da823b27690ff5c622c7132c 100644
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -1609,6 +1609,7 @@ void SerializationDataQueue::Enqueue(SerializationData* data) {
 
 bool SerializationDataQueue::Dequeue(SerializationData** data) {
   base::LockGuard<base::Mutex> lock_guard(&mutex_);
+  *data = NULL;
   if (data_.is_empty()) return false;
   *data = data_.Remove(0);
   return true;
@@ -1662,7 +1663,9 @@ void Worker::PostMessage(SerializationData* data) {
 SerializationData* Worker::GetMessage() {
   SerializationData* data = NULL;
   while (!out_queue_.Dequeue(&data)) {
-    if (base::NoBarrier_Load(&state_) != RUNNING) break;
+    // If the worker is no longer running, and there are no messages in the
+    // queue, don't expect any more messages from it.
+    if (base::NoBarrier_Load(&state_) > RUNNING) break;
     out_semaphore_.Wait();
   }
 
@@ -1671,14 +1674,53 @@ SerializationData* Worker::GetMessage() {
 
 
 void Worker::Terminate() {
-  if (base::NoBarrier_CompareAndSwap(&state_, RUNNING, TERMINATED) == RUNNING) {
+  if (base::NoBarrier_CompareAndSwap(&state_, RUNNING, TERMINATING) ==
+      RUNNING) {
     // Post NULL to wake the Worker thread message loop.
     PostMessage(NULL);
-    thread_->Join();
   }
 }
 
 
+void Worker::WaitForThread() {
+  Terminate();
+
+  State expected_state = TERMINATING;
+  while (true) {
+    State actual_state = static_cast<State>(
+        base::NoBarrier_CompareAndSwap(&state_, expected_state, JOINING));
+
+    if (actual_state == expected_state) break;
+
+    switch (actual_state) {
+      case IDLE:
+      case RUNNING:
+        // We terminated above, we should never be IDLE or RUNNING.
+        UNREACHABLE();
+        return;
+
+      case TERMINATING:
+      case STOPPED:

[Jarin, 2015/07/10 10:43:17]

Why do we want to retry when STOPPED?

Actually, I am even confused why we are trying to go through the TERMINATING
state in WaitForThread. Why (instead of calling Terminate) don't you CAS
RUNNING->JOINING?

It would be helpful if you could describe what was wrong with the original state
machine.

[binji, 2015/07/10 17:40:46]

You're right, this is too complicated. I refactored this into a much simpler
change.

Basically, the problem was that the pthread object would leak if
Worker.terminate() was never called. Really all that needed to be done was call
base::Thread::Join(), but that crashes if you call it twice. I wanted to catch
that programmer error, but adding it to the state_ variable made it too complex.

+        // Try again.
+        expected_state = actual_state;
+        break;
+
+      case JOINING:

[Jarin, 2015/07/10 10:43:17]

Is not this only called when the shell finishes, so there should not be anyone
else joining?

[binji, 2015/07/10 17:40:46]

True, this was just caution to prevent multiple joiners (which crashes). I've
refactored this into a much simpler change.

+        // Someone else is joining. Bail.
+        return;
+
+      case JOINED:
+        // Tried to join twice.
+        UNREACHABLE();
+        return;
+    }
+  }
+
+  thread_->Join();
+  base::NoBarrier_Store(&state_, JOINED);
+}
+
+
 void Worker::ExecuteInThread() {
   Isolate::CreateParams create_params;
   create_params.array_buffer_allocator = Shell::array_buffer_allocator;
@@ -1741,11 +1783,10 @@ void Worker::ExecuteInThread() {
   }
   isolate->Dispose();
 
-  if (base::NoBarrier_CompareAndSwap(&state_, RUNNING, TERMINATED) == RUNNING) {
-    // Post NULL to wake the thread waiting on GetMessage() if there is one.
-    out_queue_.Enqueue(NULL);
-    out_semaphore_.Signal();
-  }
+  base::NoBarrier_Store(&state_, STOPPED);
+  // Post NULL to wake the thread waiting on GetMessage() if there is one.
+  out_queue_.Enqueue(NULL);
+  out_semaphore_.Signal();
 }
 
 
@@ -2143,18 +2184,12 @@ MaybeLocal<Value> Shell::DeserializeValue(Isolate* isolate,
       break;
     case kSerializationTagString: {
       int length = data.Read<int>(offset);
-      static char s_buffer[128];

[binji, 2015/07/09 00:13:12]

I'm pretty ashamed of this one... :-|

-      char* p = s_buffer;
-      bool allocated = false;
-      if (length > static_cast<int>(sizeof(s_buffer))) {
-        p = new char[length];
-        allocated = true;
-      }
+      char* p = new char[length];

[Jarin, 2015/07/10 10:43:17]

How about using std::vector<char> here, instead of the naked pointer?

[binji, 2015/07/10 17:40:46]

Done (though I was not sure that std containers were allowed in this file, it
seems like they were explicitly avoided).

       data.ReadMemory(p, length, offset);
       MaybeLocal<String> str =
           String::NewFromUtf8(isolate, p, String::kNormalString, length);
       if (!str.IsEmpty()) result = str.ToLocalChecked();
-      if (allocated) delete[] p;
+      delete[] p;
       break;
     }
     case kSerializationTagArray: {
@@ -2226,7 +2261,7 @@ void Shell::CleanupWorkers() {
 
   for (int i = 0; i < workers_copy.length(); ++i) {
     Worker* worker = workers_copy[i];
-    worker->Terminate();
+    worker->WaitForThread();
     delete worker;
   }