[v8-dev] d8: Fix some TSAN bugs (issue 1226143003 by binji@chromium.org)

[binji]

Reviewers: jarin,

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc
File src/d8.cc (left):

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#oldcode2146
src/d8.cc:2146: static char s_buffer[128];
I'm pretty ashamed of this one... :-|

Description:
d8: Fix some TSAN bugs

* Fix embarrassing bug in DeserializeValue, using a static buffer in
multithreaded code.
* Fix thread leak when Worker.terminate() is not called.

R=ja...@chromium.org

Please review this at https://codereview.chromium.org/1226143003/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+54, -18 lines):
  M src/d8.h
  M 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:
+        // Try again.
+        expected_state = actual_state;
+        break;
+
+      case JOINING:
+        // 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];
-      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];
       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;
   }

Index: src/d8.h
diff --git a/src/d8.h b/src/d8.h
index  
4d723473ea385c2c66b65c6ee65fd40e872592b8..f3b8eee783f87a92912735dc567df15621300146  
100644
--- a/src/d8.h
+++ b/src/d8.h
@@ -243,6 +243,7 @@ class Worker {
   void PostMessage(SerializationData* data);
   SerializationData* GetMessage();
   void Terminate();
+  void WaitForThread();

  private:
   class WorkerThread : public base::Thread {
@@ -257,7 +258,7 @@ class Worker {
     Worker* worker_;
   };

-  enum State { IDLE, RUNNING, TERMINATED };
+  enum State { IDLE, RUNNING, TERMINATING, STOPPED, JOINING, JOINED };

   void ExecuteInThread();
   void Cleanup();


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.