Skip to content

V
V8
Commit 9281d9b6 authored by machenbach's avatar machenbach Committed by Commit bot
Browse files
Options

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

Revert of Fix bug when transferring SharedArrayBuffer to multiple Workers. (patchset #3 id:40001 of https://codereview.chromium.org/1215233004/)

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}
parent 3c272e19
Hide whitespace changes
Inline Side-by-side
Showing with 55 additions and 108 deletions
src/d8.cc
View file @ 9281d9b6
......@@ -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: {
......
src/d8.h
View file @ 9281d9b6
......@@ -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;
};
......
test/mjsunit/d8-worker-sharedarraybuffer.js
View file @ 9281d9b6
......@@ -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.
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment