diff --git a/src/api/environment.cc b/src/api/environment.cc index 459830a2af80..ff116d40d127 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -312,6 +312,34 @@ IsolateGroup GetOrCreateIsolateGroup() { return IsolateGroup::GetDefault(); } +// V8 shares the read-only heap between isolates and requires them all to be +// created from the same snapshot, so every isolate gets the blob and external +// references the first NewIsolate() call used. ~SnapshotData() leaves that blob +// alone because its owner may be gone before the last isolate is created. +static Mutex first_snapshot_mutex; +static bool first_snapshot_recorded = false; +static v8::StartupData first_snapshot_blob{nullptr, 0}; +static const intptr_t* first_external_references = nullptr; + +static void UseFirstSnapshot(Isolate::CreateParams* params) { + Mutex::ScopedLock lock(first_snapshot_mutex); + if (!first_snapshot_recorded) { + first_snapshot_recorded = true; + if (params->snapshot_blob != nullptr) { + first_snapshot_blob = *params->snapshot_blob; + } + first_external_references = params->external_references; + } + params->snapshot_blob = + first_snapshot_blob.data != nullptr ? &first_snapshot_blob : nullptr; + params->external_references = first_external_references; +} + +bool IsFirstSnapshotBlob(const char* data) { + Mutex::ScopedLock lock(first_snapshot_mutex); + return first_snapshot_recorded && data == first_snapshot_blob.data; +} + // TODO(joyeecheung): we may want to expose this, but then we need to be // careful about what we override in the params. Isolate* NewIsolate(Isolate::CreateParams* params, @@ -327,15 +355,7 @@ Isolate* NewIsolate(Isolate::CreateParams* params, SnapshotBuilder::InitializeIsolateParams(snapshot_data, params); } - { - // Because it uses a shared readonly-heap, V8 requires all snapshots used - // for creating Isolates to be identical. This isn't really memory-safe - // but also otherwise just doesn't work, and the only real alternative - // is disabling shared-readonly-heap mode altogether. - static Isolate::CreateParams first_params = *params; - params->snapshot_blob = first_params.snapshot_blob; - params->external_references = first_params.external_references; - } + UseFirstSnapshot(params); // Register the isolate on the platform before the isolate gets initialized, // so that the isolate can access the platform during initialization. diff --git a/src/node.h b/src/node.h index 8e7d1e6a2516..391ec637a3d9 100644 --- a/src/node.h +++ b/src/node.h @@ -1004,6 +1004,9 @@ class NODE_EXTERN CommonEnvironmentSetup { // will be empty. // env_args will be passed through as arguments to CreateEnvironment(), after // `isolate_data` and `context`. + // `snapshot_data` has to stay alive as long as the setup created from it, + // and every setup in a process has to use the same snapshot: all isolates + // are created from the blob the first one used. template static std::unique_ptr Create( MultiIsolatePlatform* platform, diff --git a/src/node_internals.h b/src/node_internals.h index 17e23ce61d64..cf3ce9e13f4d 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -348,6 +348,8 @@ void DefineZlibConstants(v8::Local target); // addresses, so this should be used with care. v8::IsolateGroup GetOrCreateIsolateGroup(); +// The blob every isolate is created from, see NewIsolate(). It is never freed. +bool IsFirstSnapshotBlob(const char* data); v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params, uv_loop_t* event_loop, MultiIsolatePlatform* platform, diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index e861e499534c..2291586d4eca 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -701,7 +701,8 @@ bool SnapshotData::Check() const { SnapshotData::~SnapshotData() { if (data_ownership == DataOwnership::kOwned && - v8_snapshot_blob_data.data != nullptr) { + v8_snapshot_blob_data.data != nullptr && + !IsFirstSnapshotBlob(v8_snapshot_blob_data.data)) { delete[] v8_snapshot_blob_data.data; } } diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc index 007781754ab9..8f94e6e910cb 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -116,8 +116,20 @@ NODE_MAIN(int argc, node::argv_type raw_argv[]) { cppgc::InitializeProcess(platform->GetPageAllocator()); V8::Initialize(); - int ret = - RunNodeInstance(platform.get(), result->args(), result->exec_args()); + // --embedder-run-twice: two sequential instances in one process, each + // loading (and freeing) its own copy of the snapshot. + std::vector instance_args = result->args(); + auto twice = std::find( + instance_args.begin(), instance_args.end(), "--embedder-run-twice"); + int runs = 1; + if (twice != instance_args.end()) { + instance_args.erase(twice); + runs = 2; + } + int ret = 0; + for (int i = 0; i < runs && ret == 0; i++) { + ret = RunNodeInstance(platform.get(), instance_args, result->exec_args()); + } V8::Dispose(); V8::DisposePlatform(); diff --git a/test/embedding/test-embedding-snapshot-twice.js b/test/embedding/test-embedding-snapshot-twice.js new file mode 100644 index 000000000000..7d09aab78e10 --- /dev/null +++ b/test/embedding/test-embedding-snapshot-twice.js @@ -0,0 +1,39 @@ +'use strict'; + +// Tests that an embedder can free the EmbedderSnapshotData it created an +// instance from and create a second instance from a fresh copy afterwards. + +const common = require('../common'); +const assert = require('assert'); +const tmpdir = require('../common/tmpdir'); +const fixtures = require('../common/fixtures'); +const { + spawnSyncAndAssert, + spawnSyncAndExitWithoutError, +} = require('../common/child_process'); + +const embedtest = common.resolveBuiltBinary('embedtest'); +const snapshotFixture = fixtures.path('snapshot', 'echo-args.js'); +const blob = tmpdir.resolve('embedder-snapshot.blob'); + +tmpdir.refresh(); + +spawnSyncAndExitWithoutError( + embedtest, + [ + '--', + `eval(require("fs").readFileSync(${JSON.stringify(snapshotFixture)}, "utf8"))`, + 'arg1', 'arg2', '--embedder-snapshot-blob', blob, '--embedder-snapshot-create', + ], + { cwd: tmpdir.path }); + +spawnSyncAndAssert( + embedtest, + ['--', 'arg3', '--embedder-snapshot-blob', blob, '--embedder-run-twice'], + { cwd: tmpdir.path }, + { + stdout(output) { + assert.strictEqual(output.split('arg3').length, 3); + return true; + }, + });