d8 workers: Fix transferring SharedArrayBuffer to multiple Workers. (try 2)

Note: the previous try was reverted for occasional flaky tests. This continued
after the revert, and should be fixed by
https://codereview.chromium.org/1226143003.

Previously, the serialization code would call Externalize for every transferred
ArrayBuffer or SharedArrayBuffer, but that function can only be called once. If
the buffer is already externalized, we should call GetContents instead.

Also fix use-after-free bug when transferring ArrayBuffers. The transferred
ArrayBuffer must be internalized in the new isolate, or be managed by the
Shell. The current code gives it to the isolate externalized and frees it
immediately afterward when the SerializationData object is destroyed.

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

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

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

src/d8.cc

@@ -1537,22 +1537,26 @@ void SourceGroup::WaitForThread() {
 
 
 SerializationData::~SerializationData() {
-  // Any ArrayBuffer::Contents are owned by this SerializationData object.
-  // SharedArrayBuffer::Contents may be used by other threads, so must be
+  // Any ArrayBuffer::Contents are owned by this SerializationData object if
+  // ownership hasn't been transferred out via ReadArrayBufferContents.
+  // SharedArrayBuffer::Contents may be used by multiple threads, so must be
   // cleaned up by the main thread in Shell::CleanupWorkers().
-  for (int i = 0; i < array_buffer_contents.length(); ++i) {
-    ArrayBuffer::Contents& contents = array_buffer_contents[i];
-    Shell::array_buffer_allocator->Free(contents.Data(), contents.ByteLength());
+  for (int i = 0; i < array_buffer_contents_.length(); ++i) {
+    ArrayBuffer::Contents& contents = array_buffer_contents_[i];
+    if (contents.Data()) {
+      Shell::array_buffer_allocator->Free(contents.Data(),
+                                          contents.ByteLength());
+    }
   }
 }
 
 
-void SerializationData::WriteTag(SerializationTag tag) { data.Add(tag); }
+void SerializationData::WriteTag(SerializationTag tag) { data_.Add(tag); }
 
 
 void SerializationData::WriteMemory(const void* p, int length) {
   if (length > 0) {
-    i::Vector<uint8_t> block = data.AddBlock(0, length);
+    i::Vector<uint8_t> block = data_.AddBlock(0, length);
     memcpy(&block[0], p, length);
   }
 }
@@ -1560,18 +1564,18 @@ void SerializationData::WriteMemory(const void* p, int length) {
 
 void SerializationData::WriteArrayBufferContents(
     const ArrayBuffer::Contents& contents) {
-  array_buffer_contents.Add(contents);
+  array_buffer_contents_.Add(contents);
   WriteTag(kSerializationTagTransferredArrayBuffer);
-  int index = array_buffer_contents.length() - 1;
+  int index = array_buffer_contents_.length() - 1;
   Write(index);
 }
 
 
 void SerializationData::WriteSharedArrayBufferContents(
     const SharedArrayBuffer::Contents& contents) {
-  shared_array_buffer_contents.Add(contents);
+  shared_array_buffer_contents_.Add(contents);
   WriteTag(kSerializationTagTransferredSharedArrayBuffer);
-  int index = shared_array_buffer_contents.length() - 1;
+  int index = shared_array_buffer_contents_.length() - 1;
   Write(index);
 }
 
@@ -1583,7 +1587,7 @@ SerializationTag SerializationData::ReadTag(int* offset) const {
 
 void SerializationData::ReadMemory(void* p, int length, int* offset) const {
   if (length > 0) {
-    memcpy(p, &data[*offset], length);
+    memcpy(p, &data_[*offset], length);
     (*offset) += length;
   }
 }
@@ -1592,16 +1596,20 @@ void SerializationData::ReadMemory(void* p, int length, int* offset) const {
 void SerializationData::ReadArrayBufferContents(ArrayBuffer::Contents* contents,
                                                 int* offset) const {
   int index = Read<int>(offset);
-  DCHECK(index < array_buffer_contents.length());
-  *contents = array_buffer_contents[index];
+  DCHECK(index < array_buffer_contents_.length());
+  *contents = array_buffer_contents_[index];
+  // Ownership of this ArrayBuffer::Contents is passed to the caller. Neuter
+  // our copy so it won't be double-free'd when this SerializationData is
+  // destroyed.
+  array_buffer_contents_[index] = ArrayBuffer::Contents();
 }
 
 
 void SerializationData::ReadSharedArrayBufferContents(
     SharedArrayBuffer::Contents* contents, int* offset) const {
   int index = Read<int>(offset);
-  DCHECK(index < shared_array_buffer_contents.length());
-  *contents = shared_array_buffer_contents[index];
+  DCHECK(index < shared_array_buffer_contents_.length());
+  *contents = shared_array_buffer_contents_[index];
 }
 
 
@@ -1644,7 +1652,14 @@ Worker::Worker()
       join_called_(false) {}
 
 
-Worker::~Worker() { Cleanup(); }
+Worker::~Worker() {
+  delete thread_;
+  thread_ = NULL;
+  delete[] script_;
+  script_ = NULL;
+  in_queue_.Clear();
+  out_queue_.Clear();
+}
 
 
 void Worker::StartExecuteInThread(Isolate* isolate, const char* script) {
@@ -1673,7 +1688,6 @@ SerializationData* Worker::GetMessage() {
     if (base::NoBarrier_Load(&state_) != RUNNING) break;
     out_semaphore_.Wait();
   }
-
   return data;
 }
 
@@ -1765,16 +1779,6 @@ void Worker::ExecuteInThread() {
 }
 
 
-void Worker::Cleanup() {
-  delete thread_;
-  thread_ = NULL;
-  delete[] script_;
-  script_ = NULL;
-  in_queue_.Clear();
-  out_queue_.Clear();
-}
-
-
 void Worker::PostMessageOut(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
@@ -2059,7 +2063,9 @@ bool Shell::SerializeValue(Isolate* isolate, Handle<Value> value,
         return false;
       }
 
-      ArrayBuffer::Contents contents = array_buffer->Externalize();
+      ArrayBuffer::Contents contents = array_buffer->IsExternal()
+                                           ? array_buffer->GetContents()
+                                           : array_buffer->Externalize();
       array_buffer->Neuter();
       out_data->WriteArrayBufferContents(contents);
     } else {
@@ -2088,9 +2094,14 @@ bool Shell::SerializeValue(Isolate* isolate, Handle<Value> value,
       return false;
     }
 
-    SharedArrayBuffer::Contents contents = sab->Externalize();
-    out_data->WriteSharedArrayBufferContents(contents);
+    SharedArrayBuffer::Contents contents;
+    if (sab->IsExternal()) {
+      contents = sab->GetContents();
+    } else {
+      contents = sab->Externalize();
       externalized_shared_contents_.Add(contents);
+    }
+    out_data->WriteSharedArrayBufferContents(contents);
   } else if (value->IsObject()) {
     Handle<Object> object = Handle<Object>::Cast(value);
     if (FindInObjectList(object, *seen_objects)) {
@@ -2203,8 +2214,8 @@ MaybeLocal<Value> Shell::DeserializeValue(Isolate* isolate,
     case kSerializationTagTransferredArrayBuffer: {
       ArrayBuffer::Contents contents;
       data.ReadArrayBufferContents(&contents, offset);
-      result =
-          ArrayBuffer::New(isolate, contents.Data(), contents.ByteLength());
+      result = ArrayBuffer::New(isolate, contents.Data(), contents.ByteLength(),
+                                ArrayBufferCreationMode::kInternalized);
       break;
     }
     case kSerializationTagTransferredSharedArrayBuffer: {

src/d8.h

@@ -214,9 +214,9 @@ class SerializationData {
   }
 
  private:
-  i::List<uint8_t> data;
-  i::List<ArrayBuffer::Contents> array_buffer_contents;
-  i::List<SharedArrayBuffer::Contents> shared_array_buffer_contents;
+  i::List<uint8_t> data_;
+  i::List<ArrayBuffer::Contents> array_buffer_contents_;
+  i::List<SharedArrayBuffer::Contents> shared_array_buffer_contents_;
 };
 
 
@@ -273,7 +273,6 @@ class Worker {
   };
 
   void ExecuteInThread();
-  void Cleanup();
   static void PostMessageOut(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   base::Semaphore in_semaphore_;

test/mjsunit/d8-worker-sharedarraybuffer.js

@@ -27,7 +27,10 @@
 
 // Flags: --harmony-sharedarraybuffer --harmony-atomics
 
-var workerScript =
+if (this.Worker) {
+
+  (function TestTransfer() {
+    var workerScript =
       `onmessage = function(m) {
        var sab = m;
        var ta = new Uint32Array(sab);
@@ -43,7 +46,6 @@ var workerScript =
         Atomics.store(ta, 0, 100);
       };`;
 
-if (this.Worker) {
     var w = new Worker(workerScript);
 
     var sab = new SharedArrayBuffer(16);
@@ -65,4 +67,40 @@ if (this.Worker) {
     w.terminate();
 
     assertEquals(16, sab.byteLength);  // Still not neutered.
+  })();
+
+  (function TestTransferMulti() {
+    var workerScript =
+      `onmessage = function(msg) {
+       var sab = msg.sab;
+       var id = msg.id;
+       var ta = new Uint32Array(sab);
+       Atomics.store(ta, id, 1);
+       postMessage(id);
+      };`;
+
+    var sab = new SharedArrayBuffer(16);
+    var ta = new Uint32Array(sab);
+
+    var id;
+    var workers = [];
+    for (id = 0; id < 4; ++id) {
+      workers[id] = new Worker(workerScript);
+      workers[id].postMessage({sab: sab, id: id}, [sab]);
+    }
+
+    // Spinwait for each worker to update ta[id]
+    var count = 0;
+    while (count < 4) {
+      for (id = 0; id < 4; ++id) {
+        if (Atomics.compareExchange(ta, id, 1, -1) == 1) {
+          // Worker is finished.
+          assertEquals(id, workers[id].getMessage());
+          workers[id].terminate();
+          count++;
+        }
+      }
+    }
+  })();
+
 }