[d8 Workers] Fix bug creating Worker during main thread termination

src/d8.cc

Index: src/d8.cc
diff --git a/src/d8.cc b/src/d8.cc
index a56d029f5a5fb2fedb3fb568578ca9b90f53bada..ea133048b9d7e33eadf2120691ee103773f6831c 100644
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -731,10 +731,15 @@ void Shell::WorkerNew(const v8::FunctionCallbackInfo<v8::Value>& args) {
 
   {
     base::LockGuard<base::Mutex> lock_guard(workers_mutex_.Pointer());
+    // Intialize the internal field to NULL; if we return early without
+    // creating a new Worker (because the main thread is terminating) we can
+    // early-out from the instance calls.
+    args.Holder()->SetAlignedPointerInInternalField(0, NULL);
+
     if (!allow_new_workers_) return;
 
     Worker* worker = new Worker;
-    args.This()->SetInternalField(0, External::New(isolate, worker));
+    args.Holder()->SetAlignedPointerInInternalField(0, worker);
     workers_.Add(worker);
 
     String::Utf8Value script(args[0]);
@@ -751,23 +756,23 @@ void Shell::WorkerPostMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
   Local<Context> context = isolate->GetCurrentContext();
-  Local<Value> this_value;
 
   if (args.Length() < 1) {
     Throw(isolate, "Invalid argument");
     return;
   }
 
-  if (args.This()->InternalFieldCount() > 0) {
-    this_value = args.This()->GetInternalField(0);
-  }
-  if (this_value.IsEmpty()) {
+  if (args.Holder()->InternalFieldCount() <= 0) {
     Throw(isolate, "this is not a Worker");
     return;
   }
 
-  Worker* worker =
-      static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
+  Worker* worker = static_cast<Worker*>(
+      args.Holder()->GetAlignedPointerFromInternalField(0));
+  if (worker == NULL) {
+    Throw(isolate, "Worker is defunct because main thread is terminating");
+    return;
+  }
 
   Local<Value> message = args[0];
   ObjectList to_transfer;
@@ -807,17 +812,17 @@ void Shell::WorkerPostMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
 void Shell::WorkerGetMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
-  Local<Value> this_value;
-  if (args.This()->InternalFieldCount() > 0) {
-    this_value = args.This()->GetInternalField(0);
-  }
-  if (this_value.IsEmpty()) {
+  if (args.Holder()->InternalFieldCount() <= 0) {
     Throw(isolate, "this is not a Worker");
     return;
   }
 
-  Worker* worker =
-      static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
+  Worker* worker = static_cast<Worker*>(
+      args.Holder()->GetAlignedPointerFromInternalField(0));
+  if (worker == NULL) {
+    Throw(isolate, "Worker is defunct because main thread is terminating");
+    return;
+  }
 
   SerializationData* data = worker->GetMessage();
   if (data) {
@@ -834,17 +839,18 @@ void Shell::WorkerGetMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
 void Shell::WorkerTerminate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
-  Local<Value> this_value;
-  if (args.This()->InternalFieldCount() > 0) {
-    this_value = args.This()->GetInternalField(0);
-  }
-  if (this_value.IsEmpty()) {
+  if (args.Holder()->InternalFieldCount() <= 0) {
     Throw(isolate, "this is not a Worker");
     return;
   }
 
-  Worker* worker =
-      static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
+  Worker* worker = static_cast<Worker*>(
+      args.Holder()->GetAlignedPointerFromInternalField(0));
+  if (worker == NULL) {
+    Throw(isolate, "Worker is defunct because main thread is terminating");
+    return;
+  }
+
   worker->Terminate();
 }
 #endif  // !V8_SHARED
@@ -1259,18 +1265,26 @@ Local<ObjectTemplate> Shell::CreateGlobalTemplate(Isolate* isolate) {
 
   Local<FunctionTemplate> worker_fun_template =
       FunctionTemplate::New(isolate, WorkerNew);
+  Local<Signature> worker_signature =
+      Signature::New(isolate, worker_fun_template);
+  worker_fun_template->SetClassName(
+      String::NewFromUtf8(isolate, "Worker", NewStringType::kNormal)
+          .ToLocalChecked());
   worker_fun_template->PrototypeTemplate()->Set(
       String::NewFromUtf8(isolate, "terminate", NewStringType::kNormal)
           .ToLocalChecked(),
-      FunctionTemplate::New(isolate, WorkerTerminate));
+      FunctionTemplate::New(isolate, WorkerTerminate, Local<Value>(),
+                            worker_signature));
   worker_fun_template->PrototypeTemplate()->Set(
       String::NewFromUtf8(isolate, "postMessage", NewStringType::kNormal)
           .ToLocalChecked(),
-      FunctionTemplate::New(isolate, WorkerPostMessage));
+      FunctionTemplate::New(isolate, WorkerPostMessage, Local<Value>(),
+                            worker_signature));
   worker_fun_template->PrototypeTemplate()->Set(
       String::NewFromUtf8(isolate, "getMessage", NewStringType::kNormal)
           .ToLocalChecked(),
-      FunctionTemplate::New(isolate, WorkerGetMessage));
+      FunctionTemplate::New(isolate, WorkerGetMessage, Local<Value>(),
+                            worker_signature));
   worker_fun_template->InstanceTemplate()->SetInternalFieldCount(1);
   global_template->Set(
       String::NewFromUtf8(isolate, "Worker", NewStringType::kNormal)
@@ -1782,8 +1796,7 @@ Worker::Worker()
       out_semaphore_(0),
       thread_(NULL),
       script_(NULL),
-      state_(IDLE),
-      join_called_(false) {}
+      state_(IDLE) {}
 
 
 Worker::~Worker() {
@@ -1797,14 +1810,12 @@ Worker::~Worker() {
 
 
 void Worker::StartExecuteInThread(Isolate* isolate, const char* script) {
-  if (base::NoBarrier_CompareAndSwap(&state_, IDLE, RUNNING) == IDLE) {
-    script_ = i::StrDup(script);
-    thread_ = new WorkerThread(this);
-    thread_->Start();
-  } else {
-    // Somehow the Worker was started twice.
-    UNREACHABLE();
-  }
+  DCHECK(state_ == IDLE);
+  state_ = RUNNING;
+
+  script_ = i::StrDup(script);
+  thread_ = new WorkerThread(this);
+  thread_->Start();
 }
 
 
@@ -1819,7 +1830,7 @@ SerializationData* Worker::GetMessage() {
   while (!out_queue_.Dequeue(&data)) {
     // 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;
+    if (state_ != RUNNING) break;
     out_semaphore_.Wait();
   }
   return data;
@@ -1827,7 +1838,8 @@ SerializationData* Worker::GetMessage() {
 
 
 void Worker::Terminate() {
-  if (base::NoBarrier_CompareAndSwap(&state_, RUNNING, TERMINATED) == RUNNING) {
+  if (state_ == RUNNING) {
+    state_ = TERMINATED;

[Jarin, 2015/07/23 07:48:41]

What if we call quit() from one worker and worker.terminate() from another one?
Cannot there be a data race?

[binji, 2015/07/24 18:52:29]

Yes, you're right. I've removed the state_ (because it was mostly for
debugging), and replaced it with a simple atomic bool.

     // Post NULL to wake the Worker thread message loop, and tell it to stop
     // running.
     PostMessage(NULL);
@@ -1837,13 +1849,7 @@ void Worker::Terminate() {
 
 void Worker::WaitForThread() {
   Terminate();
-
-  if (base::NoBarrier_CompareAndSwap(&join_called_, false, true) == false) {
-    thread_->Join();
-  } else {
-    // Tried to call join twice.
-    UNREACHABLE();
-  }
+  thread_->Join();
 }