From 5bebab19927f861398e351b3d77cf277308a8022 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 1 Jan 2019 21:42:43 +0100 Subject: [PATCH 1/3] worker: use engine-provided deleter for `SharedArrayBuffer`s Store the full information we have on a given `SharedArrayBuffer`, and use the deleter provided by the JS engine to free the memory when that is needed. This fixes memory lifetime management for WASM buffers that are passed through a `MessageChannel` (e.g. between threads). --- src/sharedarraybuffer_metadata.cc | 16 +++++++---- src/sharedarraybuffer_metadata.h | 5 ++-- test/fixtures/wasm-threads-shared-memory.wasm | Bin 0 -> 36 bytes test/fixtures/wasm-threads-shared-memory.wat | 4 +++ .../test-worker-message-port-wasm-threads.js | 26 ++++++++++++++++++ 5 files changed, 42 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/wasm-threads-shared-memory.wasm create mode 100644 test/fixtures/wasm-threads-shared-memory.wat create mode 100644 test/parallel/test-worker-message-port-wasm-threads.js diff --git a/src/sharedarraybuffer_metadata.cc b/src/sharedarraybuffer_metadata.cc index 3e760bc50bebe6..671ad6d6820440 100644 --- a/src/sharedarraybuffer_metadata.cc +++ b/src/sharedarraybuffer_metadata.cc @@ -89,8 +89,7 @@ SharedArrayBufferMetadata::ForSharedArrayBuffer( } SharedArrayBuffer::Contents contents = source->Externalize(); - SharedArrayBufferMetadataReference r(new SharedArrayBufferMetadata( - contents.Data(), contents.ByteLength())); + SharedArrayBufferMetadataReference r(new SharedArrayBufferMetadata(contents)); if (r->AssignToSharedArrayBuffer(env, context, source).IsNothing()) return nullptr; return r; @@ -111,17 +110,22 @@ Maybe SharedArrayBufferMetadata::AssignToSharedArrayBuffer( obj); } -SharedArrayBufferMetadata::SharedArrayBufferMetadata(void* data, size_t size) - : data(data), size(size) { } +SharedArrayBufferMetadata::SharedArrayBufferMetadata( + const SharedArrayBuffer::Contents& contents) + : contents_(contents) { } SharedArrayBufferMetadata::~SharedArrayBufferMetadata() { - free(data); + contents_.Deleter()(contents_.Data(), + contents_.ByteLength(), + contents_.DeleterData()); } MaybeLocal SharedArrayBufferMetadata::GetSharedArrayBuffer( Environment* env, Local context) { Local obj = - SharedArrayBuffer::New(env->isolate(), data, size); + SharedArrayBuffer::New(env->isolate(), + contents_.Data(), + contents_.ByteLength()); if (AssignToSharedArrayBuffer(env, context, obj).IsNothing()) return MaybeLocal(); diff --git a/src/sharedarraybuffer_metadata.h b/src/sharedarraybuffer_metadata.h index 84bfd224fabcf8..8c753a89c11a88 100644 --- a/src/sharedarraybuffer_metadata.h +++ b/src/sharedarraybuffer_metadata.h @@ -46,7 +46,7 @@ class SharedArrayBufferMetadata SharedArrayBufferMetadata(const SharedArrayBufferMetadata&) = delete; private: - explicit SharedArrayBufferMetadata(void* data, size_t size); + explicit SharedArrayBufferMetadata(const v8::SharedArrayBuffer::Contents&); // Attach a lifetime tracker object with a reference count to `target`. v8::Maybe AssignToSharedArrayBuffer( @@ -54,8 +54,7 @@ class SharedArrayBufferMetadata v8::Local context, v8::Local target); - void* data = nullptr; - size_t size = 0; + v8::SharedArrayBuffer::Contents contents_; }; } // namespace worker diff --git a/test/fixtures/wasm-threads-shared-memory.wasm b/test/fixtures/wasm-threads-shared-memory.wasm new file mode 100644 index 0000000000000000000000000000000000000000..24b4d691058fd4f8987fb45e5ff5b9069ff1c36d GIT binary patch literal 36 rcmZQbEY4+QU|?WnVPs}xV&`IH%T3MAFREl>VBlcMOUzAWVq^dSU { + // Make sure serialized + deserialized buffer refer to the same memory. + const expected = 'Hello, world!'; + const bytes = Buffer.from(buffer).write(expected); + const deserialized = Buffer.from(buffer2).toString('utf8', 0, bytes); + assert.deepStrictEqual(deserialized, expected); +})); From c6dbe1a709deaf0fea5f22c904613fd88706e989 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 2 Jan 2019 16:04:48 +0100 Subject: [PATCH 2/3] fixup! worker: use engine-provided deleter for `SharedArrayBuffer`s --- .../test-worker-message-port-wasm-threads.js | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-worker-message-port-wasm-threads.js b/test/parallel/test-worker-message-port-wasm-threads.js index a1c58861ac1605..cdbf896f760430 100644 --- a/test/parallel/test-worker-message-port-wasm-threads.js +++ b/test/parallel/test-worker-message-port-wasm-threads.js @@ -2,7 +2,7 @@ 'use strict'; const common = require('../common'); const assert = require('assert'); -const { MessageChannel } = require('worker_threads'); +const { MessageChannel, Worker } = require('worker_threads'); // Test that SharedArrayBuffer instances created from WASM are transferrable // through MessageChannels (without crashing). @@ -15,12 +15,33 @@ const instance = new WebAssembly.Instance(wasmModule); const { buffer } = instance.exports.memory; assert(buffer instanceof SharedArrayBuffer); -const { port1, port2 } = new MessageChannel(); -port1.postMessage(buffer); -port2.once('message', common.mustCall((buffer2) => { - // Make sure serialized + deserialized buffer refer to the same memory. - const expected = 'Hello, world!'; - const bytes = Buffer.from(buffer).write(expected); - const deserialized = Buffer.from(buffer2).toString('utf8', 0, bytes); - assert.deepStrictEqual(deserialized, expected); -})); +{ + const { port1, port2 } = new MessageChannel(); + port1.postMessage(buffer); + port2.once('message', common.mustCall((buffer2) => { + // Make sure serialized + deserialized buffer refer to the same memory. + const expected = 'Hello, world!'; + const bytes = Buffer.from(buffer).write(expected); + const deserialized = Buffer.from(buffer2).toString('utf8', 0, bytes); + assert.deepStrictEqual(deserialized, expected); + })); +} + +{ + // Make sure we can free WASM memory originating from a thread that already + // stopped when we exit. + const worker = new Worker(` + const { parentPort } = require('worker_threads'); + const fixtures = require('${__dirname}/../common/fixtures'); + const wasmSource = fixtures.readSync('wasm-threads-shared-memory.wasm'); + const wasmModule = new WebAssembly.Module(wasmSource); + const instance = new WebAssembly.Instance(wasmModule); + parentPort.postMessage(instance.exports.memory); + `, { eval: true }); + worker.once('message', common.mustCall(({ buffer }) => { + assert(buffer instanceof SharedArrayBuffer); + worker.buf = buffer; // Basically just keep the reference to buffer alive. + })); + worker.once('exit', common.mustCall()); + worker.postMessage({ wasmModule }); +} From c35ec275b2162055757c0bce951c3bc62bdc20b7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 3 Jan 2019 01:23:46 +0100 Subject: [PATCH 3/3] fixup! fixup! worker: use engine-provided deleter for `SharedArrayBuffer`s --- test/parallel/test-worker-message-port-wasm-threads.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/parallel/test-worker-message-port-wasm-threads.js b/test/parallel/test-worker-message-port-wasm-threads.js index cdbf896f760430..6d4f21d728d1b3 100644 --- a/test/parallel/test-worker-message-port-wasm-threads.js +++ b/test/parallel/test-worker-message-port-wasm-threads.js @@ -32,8 +32,7 @@ assert(buffer instanceof SharedArrayBuffer); // stopped when we exit. const worker = new Worker(` const { parentPort } = require('worker_threads'); - const fixtures = require('${__dirname}/../common/fixtures'); - const wasmSource = fixtures.readSync('wasm-threads-shared-memory.wasm'); + const wasmSource = new Uint8Array([${wasmSource.join(',')}]); const wasmModule = new WebAssembly.Module(wasmSource); const instance = new WebAssembly.Instance(wasmModule); parentPort.postMessage(instance.exports.memory);