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

When the main thread terminates, it forcibly terminates all Worker threads.
When this happens, the threads objects were only half-created; they had a
JavaScript Worker object, but not a C++ worker object.

This CL fixes that bug, as well as some other fixes:
* Signatures on Worker methods
* Use SetAlignedPointerFromInternalField instead of using an External.
* Remove state_ from Worker. Simplify to atomic bool running_.

BUG=chromium:511880
R=jarin@chromium.org
LOG=n

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

Cr-Commit-Position: refs/heads/master@{#29911}

diff --git a/src/d8.cc b/src/d8.cc
index 9aeac11..6dee196 100644 (file)
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -105,6 +105,13 @@ class MockArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
 v8::Platform* g_platform = NULL;
 
 
+static Local<Value> Throw(Isolate* isolate, const char* message) {
+  return isolate->ThrowException(
+      String::NewFromUtf8(isolate, message, NewStringType::kNormal)
+          .ToLocalChecked());
+}
+
+
 #ifndef V8_SHARED
 bool FindInObjectList(Local<Object> object, const Shell::ObjectList& list) {
   for (int i = 0; i < list.length(); ++i) {
@@ -114,19 +121,28 @@ bool FindInObjectList(Local<Object> object, const Shell::ObjectList& list) {
   }
   return false;
 }
-#endif  // !V8_SHARED
 
 
-}  // namespace
+Worker* GetWorkerFromInternalField(Isolate* isolate, Local<Object> object) {
+  if (object->InternalFieldCount() != 1) {
+    Throw(isolate, "this is not a Worker");
+    return NULL;
+  }
 
+  Worker* worker =
+      static_cast<Worker*>(object->GetAlignedPointerFromInternalField(0));
+  if (worker == NULL) {
+    Throw(isolate, "Worker is defunct because main thread is terminating");
+    return NULL;
+  }
 
-static Local<Value> Throw(Isolate* isolate, const char* message) {
-  return isolate->ThrowException(
-      String::NewFromUtf8(isolate, message, NewStringType::kNormal)
-          .ToLocalChecked());
+  return worker;
 }
+#endif  // !V8_SHARED
 
 
+}  // namespace
+
 
 class PerIsolateData {
  public:
@@ -686,10 +702,15 @@ void Shell::WorkerNew(const v8::FunctionCallbackInfo<v8::Value>& args) {
 
   {
     base::LockGuard<base::Mutex> lock_guard(workers_mutex_.Pointer());
+    // Initialize 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 +718,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,24 +727,17 @@ 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()) {
-    Throw(isolate, "this is not a Worker");
+  Worker* worker = GetWorkerFromInternalField(isolate, args.Holder());
+  if (!worker) {
     return;
   }
 
-  Worker* worker =
-      static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
-
   Local<Value> message = args[0];
   ObjectList to_transfer;
   if (args.Length() >= 2) {
@@ -762,18 +776,11 @@ 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()) {
-    Throw(isolate, "this is not a Worker");
+  Worker* worker = GetWorkerFromInternalField(isolate, args.Holder());
+  if (!worker) {
     return;
   }
 
-  Worker* worker =
-      static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
-
   SerializationData* data = worker->GetMessage();
   if (data) {
     int offset = 0;
@@ -789,17 +796,11 @@ 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()) {
-    Throw(isolate, "this is not a Worker");
+  Worker* worker = GetWorkerFromInternalField(isolate, args.Holder());
+  if (!worker) {
     return;
   }
 
-  Worker* worker =
-      static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
   worker->Terminate();
 }
 #endif  // !V8_SHARED
@@ -1134,18 +1135,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 +1653,7 @@ Worker::Worker()
       out_semaphore_(0),
       thread_(NULL),
       script_(NULL),
-      state_(IDLE),
-      join_called_(false) {}
+      running_(false) {}
 
 
 Worker::~Worker() {
@@ -1658,15 +1666,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 +1685,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 +1693,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();
 }
 
 

diff --git a/src/d8.h b/src/d8.h
index 39c3b74..6c0b5a0 100644 (file)
--- a/src/d8.h
+++ b/src/d8.h
@@ -221,19 +221,25 @@ class Worker {
   Worker();
   ~Worker();
 
-  void StartExecuteInThread(Isolate* isolate, const char* script);
+  // Run the given script on this Worker. This function should only be called
+  // once, and should only be called by the thread that created the Worker.
+  void StartExecuteInThread(const char* script);
   // Post a message to the worker's incoming message queue. The worker will
   // take ownership of the SerializationData.
+  // This function should only be called by the thread that created the Worker.
   void PostMessage(SerializationData* data);
   // Synchronously retrieve messages from the worker's outgoing message queue.
   // If there is no message in the queue, block until a message is available.
   // If there are no messages in the queue and the worker is no longer running,
   // return nullptr.
+  // This function should only be called by the thread that created the Worker.
   SerializationData* GetMessage();
   // Terminate the worker's event loop. Messages from the worker that have been
   // queued can still be read via GetMessage().
+  // This function can be called by any thread.
   void Terminate();
   // Terminate and join the thread.
+  // This function can be called by any thread.
   void WaitForThread();
 
  private:
@@ -249,12 +255,6 @@ class Worker {
     Worker* worker_;
   };
 
-  enum State {
-    IDLE,       // The worker thread hasn't been started yet
-    RUNNING,    // The worker thread is running and hasn't been terminated
-    TERMINATED  // The worker thread has been terminated and will soon finish
-  };
-
   void ExecuteInThread();
   static void PostMessageOut(const v8::FunctionCallbackInfo<v8::Value>& args);
 
@@ -264,8 +264,7 @@ class Worker {
   SerializationDataQueue out_queue_;
   base::Thread* thread_;
   char* script_;
-  base::Atomic32 state_;
-  base::Atomic32 join_called_;
+  base::Atomic32 running_;
 };
 #endif  // !V8_SHARED
 

diff --git a/test/mjsunit/regress/regress-crbug-511880.js b/test/mjsunit/regress/regress-crbug-511880.js
new file mode 100644 (file)
index 0000000..f9b05ff
--- /dev/null
+++ b/test/mjsunit/regress/regress-crbug-511880.js
@@ -0,0 +1,13 @@
+// Copyright 2015 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+if (this.Worker) {
+  var __v_8 =
+    `var __v_9 = new Worker('postMessage(42)');
+     onmessage = function(parentMsg) {
+       __v_9.postMessage(parentMsg);
+     };`;
+  var __v_9 = new Worker(__v_8);
+  __v_9.postMessage(9);
+}