[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 52df5c035d38ed9d4fc42049720313f9edfd0010..d457e8376e7fa5990ef3fd0ce533d394f4f9678f 100644
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -686,10 +686,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]);
@@ -697,7 +702,7 @@ void Shell::WorkerNew(const v8::FunctionCallbackInfo<v8::Value>& args) {
       Throw(args.GetIsolate(), "Can't get worker script");
       return;
     }
-    worker->StartExecuteInThread(isolate, *script);
+    worker->StartExecuteInThread(*script);
   }
 }
 
@@ -706,23 +711,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;
@@ -762,17 +767,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;
+  }

[Jarin, 2015/07/27 05:09:28]

How about factoring out the extraction of the worker from the args into a helper
function? 

Also, could you DCHECK that InternalFielCount is 1? Just to exclude the
possibility that we deal with some stray object.

[binji, 2015/07/29 04:37:13]

Done.
DCHECK or runtime check? I'm hesitant to do a DCHECK because cluster-fuzz :)

 
   SerializationData* data = worker->GetMessage();
   if (data) {
@@ -789,17 +794,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
@@ -1134,18 +1140,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)
@@ -1644,8 +1658,7 @@ Worker::Worker()
       out_semaphore_(0),
       thread_(NULL),
       script_(NULL),
-      state_(IDLE),
-      join_called_(false) {}
+      running_(false) {}
 
 
 Worker::~Worker() {
@@ -1658,15 +1671,11 @@ 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();
-  }
+void Worker::StartExecuteInThread(const char* script) {
+  running_ = true;
+  script_ = i::StrDup(script);
+  thread_ = new WorkerThread(this);
+  thread_->Start();
 }
 
 
@@ -1681,7 +1690,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 (!base::NoBarrier_Load(&running_)) break;
     out_semaphore_.Wait();
   }
   return data;
@@ -1689,23 +1698,16 @@ SerializationData* Worker::GetMessage() {
 
 
 void Worker::Terminate() {
-  if (base::NoBarrier_CompareAndSwap(&state_, RUNNING, TERMINATED) == RUNNING) {
-    // Post NULL to wake the Worker thread message loop, and tell it to stop
-    // running.
-    PostMessage(NULL);
-  }
+  base::NoBarrier_Store(&running_, false);
+  // Post NULL to wake the Worker thread message loop, and tell it to stop
+  // running.
+  PostMessage(NULL);
 }
 
 
 void Worker::WaitForThread() {
   Terminate();
-
-  if (base::NoBarrier_CompareAndSwap(&join_called_, false, true) == false) {
-    thread_->Join();
-  } else {
-    // Tried to call join twice.
-    UNREACHABLE();
-  }
+  thread_->Join();
 }