Revert of Fix bug when transferring SharedArrayBuffer to multiple Workers. (patchset...

Reason for revert:
[Sheriff] Test hangs sometimes and times out flakily. E.g.: http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosse3/builds/4551/steps/Check%20%28flakes%29/logs/d8-worker-sharedarray..

Original issue's description:
> Fix bug when transferring SharedArrayBuffer to multiple Workers.
>
> 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
>
> Committed: https://crrev.com/dd7962bf7838f8379ba776ee6b7b0e4d3bec2140
> Cr-Commit-Position: refs/heads/master@{#29499}

TBR=jarin@chromium.org,jochen@chromium.org,binji@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=chromium:497295

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

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

diff --git a/src/d8.cc b/src/d8.cc
index a66213e..7db6f3e 100644 (file)
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -1533,26 +1533,22 @@ void SourceGroup::WaitForThread() {
 
 
 SerializationData::~SerializationData() {
-  // 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
+  // Any ArrayBuffer::Contents are owned by this SerializationData object.
+  // SharedArrayBuffer::Contents may be used by other 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];
-    if (contents.Data()) {
-      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];
+    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 +1556,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 +1579,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,20 +1588,16 @@ 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];
-  // 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();
+  DCHECK(index < array_buffer_contents.length());
+  *contents = array_buffer_contents[index];
 }
 
 
 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];
 }
 
 
@@ -2048,9 +2040,7 @@ bool Shell::SerializeValue(Isolate* isolate, Handle<Value> value,
         return false;
       }
 
-      ArrayBuffer::Contents contents = array_buffer->IsExternal()
-                                           ? array_buffer->GetContents()
-                                           : array_buffer->Externalize();
+      ArrayBuffer::Contents contents = array_buffer->Externalize();
       array_buffer->Neuter();
       out_data->WriteArrayBufferContents(contents);
     } else {
@@ -2079,14 +2069,9 @@ bool Shell::SerializeValue(Isolate* isolate, Handle<Value> value,
       return false;
     }
 
-    SharedArrayBuffer::Contents contents;
-    if (sab->IsExternal()) {
-      contents = sab->GetContents();
-    } else {
-      contents = sab->Externalize();
-      externalized_shared_contents_.Add(contents);
-    }
+    SharedArrayBuffer::Contents contents = sab->Externalize();
     out_data->WriteSharedArrayBufferContents(contents);
+    externalized_shared_contents_.Add(contents);
   } else if (value->IsObject()) {
     Handle<Object> object = Handle<Object>::Cast(value);
     if (FindInObjectList(object, *seen_objects)) {
@@ -2202,8 +2187,8 @@ MaybeLocal<Value> Shell::DeserializeValue(Isolate* isolate,
     case kSerializationTagTransferredArrayBuffer: {
       ArrayBuffer::Contents contents;
       data.ReadArrayBufferContents(&contents, offset);
-      result = ArrayBuffer::New(isolate, contents.Data(), contents.ByteLength(),
-                                ArrayBufferCreationMode::kInternalized);
+      result =
+          ArrayBuffer::New(isolate, contents.Data(), contents.ByteLength());
       break;
     }
     case kSerializationTagTransferredSharedArrayBuffer: {

diff --git a/src/d8.h b/src/d8.h
index e98d393..4d72347 100644 (file)
--- a/src/d8.h
+++ b/src/d8.h
@@ -215,9 +215,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;
 };
 
 

diff --git a/test/mjsunit/d8-worker-sharedarraybuffer.js b/test/mjsunit/d8-worker-sharedarraybuffer.js
index d432f97..791529f 100644 (file)
--- a/test/mjsunit/d8-worker-sharedarraybuffer.js
+++ b/test/mjsunit/d8-worker-sharedarraybuffer.js
@@ -27,80 +27,42 @@
 
 // Flags: --harmony-sharedarraybuffer --harmony-atomics
 
-if (this.Worker) {
-
-  (function TestTransfer() {
-    var workerScript =
-      `onmessage = function(m) {
-       var sab = m;
-       var ta = new Uint32Array(sab);
-       if (sab.byteLength !== 16) {
-         throw new Error('SharedArrayBuffer transfer byteLength');
-       }
-       for (var i = 0; i < 4; ++i) {
-         if (ta[i] !== i) {
-           throw new Error('SharedArrayBuffer transfer value ' + i);
-         }
-       }
-        // Atomically update ta[0]
-        Atomics.store(ta, 0, 100);
-      };`;
-
-    var w = new Worker(workerScript);
-
-    var sab = new SharedArrayBuffer(16);
-    var ta = new Uint32Array(sab);
-    for (var i = 0; i < 4; ++i) {
-      ta[i] = i;
-    }
+var workerScript =
+  `onmessage = function(m) {
+   var sab = m;
+   var ta = new Uint32Array(sab);
+   if (sab.byteLength !== 16) {
+     throw new Error('SharedArrayBuffer transfer byteLength');
+   }
+   for (var i = 0; i < 4; ++i) {
+     if (ta[i] !== i) {
+       throw new Error('SharedArrayBuffer transfer value ' + i);
+     }
+   }
+    // Atomically update ta[0]
+    Atomics.store(ta, 0, 100);
+  };`;
 
-    // Transfer SharedArrayBuffer
-    w.postMessage(sab, [sab]);
-    assertEquals(16, sab.byteLength);  // ArrayBuffer should not be neutered.
-
-    // Spinwait for the worker to update ta[0]
-    var ta0;
-    while ((ta0 = Atomics.load(ta, 0)) == 0) {}
-
-    assertEquals(100, ta0);
-
-    w.terminate();
+if (this.Worker) {
+  var w = new Worker(workerScript);
 
-    assertEquals(16, sab.byteLength);  // Still not neutered.
-  })();
+  var sab = new SharedArrayBuffer(16);
+  var ta = new Uint32Array(sab);
+  for (var i = 0; i < 4; ++i) {
+    ta[i] = i;
+  }
 
-  (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);
-      };`;
+  // Transfer SharedArrayBuffer
+  w.postMessage(sab, [sab]);
+  assertEquals(16, sab.byteLength);  // ArrayBuffer should not be neutered.
 
-    var sab = new SharedArrayBuffer(16);
-    var ta = new Uint32Array(sab);
+  // Spinwait for the worker to update ta[0]
+  var ta0;
+  while ((ta0 = Atomics.load(ta, 0)) == 0) {}
 
-    var id;
-    var workers = [];
-    for (id = 0; id < 4; ++id) {
-      workers[id] = new Worker(workerScript);
-      workers[id].postMessage({sab: sab, id: id}, [sab]);
-    }
+  assertEquals(100, ta0);
 
-    // 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++;
-        }
-      }
-    }
-  })();
+  w.terminate();
 
+  assertEquals(16, sab.byteLength);  // Still not neutered.
 }